How to deal with old OO code that mixes instance methods with class methods

[eyepopslikeamosquito]

I have inherited some old Perl 5.8 code. It is a straightforward, old-fashioned Perl OO module, more "object based" than "object oriented" in that there is no inheritance.

I noticed some of the "instance methods" are not really instance methods in that though they start with my $self = shift they do not actually use $self in the function body. That is, the class is a mixture of instance methods (related to the class and using object state) and utility functions (not really related to the class and not using object state). The user of the class currently calls both these different types of functions in the same way, via the object handle, for example:

my $obj = MyPackage->new("...");
# ...
my @some_list = $obj->some_method("some string");

I seem to have three options:

1. Leave the utility functions as instance methods
2. Convert them to class methods
3. Convert them to standalone utility functions

For option three above, I could leave them in the same package, or move them to a new package consisting of utility functions only. I seek your advice on how best to deal with this.

To illustrate with some sample code, here is MyPackage.pm:

package MyPackage;
use strict;
use warnings;

sub new
{
   my $class = shift;
   $class eq __PACKAGE__ or die "oops: ctor unexpected '$class'";
   my $name  = shift;
   print "in ctor: '$class' (name=$name)\n";
   my $self  = {};
   $self->{NAME} = $name;
   bless $self, $class;
   return $self;
}

sub some_instance_method
{
   my $self = shift;
   my $param1 = shift;
   print "in some_instance_method: $self '$param1'\n";
   return $self->{NAME};
}

sub some_class_method
{
   my $class = shift;
   my $param1 = shift;
   print "in some_class_method: '$class' ($param1)\n";
   $class eq __PACKAGE__ or die "oops: unexpected '$class'";
   return "hello from some class method";
}

sub some_utility_method
{
   my $param1 = shift;
   print "in some_utility_method: ($param1)\n";
   return "hello from some utility method";
}

1;

And here is a sample program that calls into this package:

use strict;
use warnings;
use MyPackage;
my $obj = MyPackage->new("John Smith");
my $name = $obj->some_instance_method(69);
print "name='$name'\n";
my $cm = MyPackage->some_class_method(42);
print "cm='$cm'\n";
my $um = MyPackage::some_utility_method(43);
print "um='$um'\n";

Running the sample program above produces:

in ctor: 'MyPackage' (name=John Smith)
in some_instance_method: MyPackage=HASH(0x1c905b0) '69'
name='John Smith'
in some_class_method: 'MyPackage' (42)
cm='hello from some class method'
in some_utility_method: (43)
um='hello from some utility method'

[GrandFather]

If it ain't broke, don't fix it. Some stuff to think about:

• Is there a compelling reason for the calling code to know about implementation details?
• Is there a compelling run time issue the change would fix?
• Is there a compelling maintenance issue the change would fix?
• Are you confident you won't introduce bugs making the change?
• Does all the affected code have unit tests?
• Is this the most important thing you can be doing right now?
• Does this change advance your carer or improve your coding ability?

Not all considerations are completely serious, but decide which are important to you, or if there are others I've missed, and think about those criteria to make your decision.

[tobyink]

The thing is, if you have a method like:

sub get_name {
   my $self = shift;
   return "Bob";
}

... it's true that this implementation of get_name doesn't make use of $self to determine the return value. However, other implementations of get_name (perhaps in child classes, parent classes, or even sibling classes; perhaps in classes that have not been written yet, but might in the future) might need to refer to $self to return a value. Making get_name a method call rather than a function call is beneficial for polymorphism.

[LanX]

If these module functions are completely unrelated to the class you could move them to another module "util" and even import them from there.

I say "could" bc it's up to you to decide which way you think is best to maintain the code in the future.

[educated_foo]

3. Convert them to standalone utility functions

Do this. It was fashionable for awhile to add more rightward arrows to your code (see e.g. File::Spec vs. File::Spec::Functions). Then the fashion was to write standalone functions that dealt with this "OO" nonsense and call them a "DSL". All of these options were terrible.

[Anonymous Monk]

Hi,

Is it working? If so, leave it alone until -

    a) you have to do something
    b) someone else inherits it from you

J.C.

[eyepopslikeamosquito]

Is it working?

Well, it is in production. :) I have been tasked with fixing a number of reported bugs. It qualifies as legacy code in that:
• It does not use strict or warnings.
• No unit tests.
• Hard to test in isolation. Not designed with testability in mind.
• No documentation.
• No coherent error handling policy.
• Lots of code duplication.
• Big-arse functions.
• Functions that are not cohesive (similar to a sin_and_tan() function).
• Inconsistent naming. Poorly chosen names.
• Lots of unnecessary cleverness.
• Code that does not match the comments.
• Pointless code (e.g. checks if running on Unix at runtime yet contains "use Win32::..." and so won't even compile on Unix).
• Constructor does not check validity of arguments.
• Module does not contain a version.
• Oh, and perl critic found a number of flaws such as: i) open calls not being checked; ii) $_ being modified in map and grep blocks; iii) capture variables being used without checking if regex succeeded.

[AnomalousMonk]

[The module has] a number of reported bugs.

Ok, so it's broke.

[The module has] no unit tests.

This is the first problem I would address: Nobody touch nothin' until you figure out a way to test what you 'fix'.

Only after you can test any changes should you address the reported bugs. After resolving any code changes necessitated by these bugs, I would consider relapsing into "no broke, no fix" mode. Of course, if you're a programmer worth your salt, the list of 'problems' you have presented will greatly tempt your intervention. The issues most likely to draw my attention would be those bearing on maintainability. You must recognize, however, that you are venturing upon a slippery slope and may, willy-nilly, find yourself in full-on "can't fix just one" mode. Caveat programmor.

[BrowserUk]

What would be really interesting (to me at least; but I think widely), would be to post the original code with sufficient context/data to allow it to be run, and solicit as many people as possible to post their reworking of it.

You could then pick'n'mix for your own solution; and the rest of us could compare our solutions with those of others.

It would be a rare opportunity for everyone, regardless of their expertise, to learn from others.

[karlgoethebier]

"I seem to have three options..."

IMHO you have one more: If it works - don't touch it!

"I have inherited some ... code"

I have just inherited some code too from a colleague who passed away some weeks ago.

OK, not Perl, it is Bash.

The guy needed about 3 years to write this stuff. Some thousands lines of code.

I had a kind of reflex to port the whole stuff to Perl.

I gave up. I'm not searching for the Perl or OO holy grale.

Pros: What works is OK.

Cons: I need to read the manpages of sed and awk again ;-(

I hope, this isn't too much off topic.

Regards, Karl

[Ovid]

It sounds like you have quite the mess on your hands. You've mentioned that it has a number of reported bugs, so leaving it be is not an option.

When I find situations like this, I generally use the following strategy.

For each bug:
1. Find the responsible module
2. Write decent, full-stack integration (not unit!) tests to expose the bug
3. If you cannot write an integration test that exposes the bug, you likely have some bad assumptions somewhere
4. Write a unit test that exposes the bug (if feasible)
5. Fix bug
6. Watch the relevant tests pass (or return to previous step until they do)

I then keep repeating the above steps for each bug. This might seem like a strange approach, but here's the rationale.

Integration tests, by their very nature, cover a lot more code and they're often your only resort if the code cannot be unit tested. Plus, they tell you if the individual components integrate (i.e., can talk to each other). The downside of integration tests is that they tend to expose more bugs, but they make it harder to find the source of the bug or to nail down the exact behavior of a single piece of code. You then fall back to a unit test (if possible) to focus on the bug. Relying on a unit test alone runs the risk of "fixing" the behavior in such a way that doesn't always work for other parts of the system (since, by their very nature, unit tests ignore integration).

By repeating these steps for each bug, you slowly start to build up an integration test suite that tends to cover the known fragile parts of the code. When you're ready to refactor something, you can start fleshing out your integration tests to cover enough code to feel moderately confident that you won't damage things too much.

It's a slow, tedious process, but refactoring legacy test suites is hard and the "better safe than sorry" approach is warranted.

As an aside, I've often found that focusing too much on unit tests means that you can waste time unit testing dead code. Worse, I see people running Devel::Cover and reporting very high code coverage over unit tests, but that obscures how much of that code can actually be called. If you put your full-stack integration tests in a separate directory, you can run your code coverage over that and when you find parts of your code aren't covered, you can then ask "am I missing tests or is this dead code?"

[CountZero]

4: Replace it with a well-tested CPAN module that does things "right" (for whatever definition of right)

[sundialsvc4]

In the third post on this thread, you listed a cook’s recipe of things that could be addressed.   All of them ideally should be, and this is an excellent priorities-list, but you can’t do them all.   (As all of us know.)   So, I would start with adding use strict; use warnings; to each module at a time and cleaning-up (under version control, of course) each issue.   I use git and would commit after each meaningful group of changes.   Basically, if there are things which would qualify as “source-code errors,” these deserve the highest attention.   I would also use a ticket-system such as Trac or Unfuddle, even “just for myself.”   Don’t trust your memory, and do have a system that links tickets to (git) changesets.   Going back, even in a sheer panic and with your bos breathing down your neck, to the known initial state of the system, is a mere git checkout away.   (“Priceless.™”)

Then, I would turn to a fairly large series of tests ... starting with, say, LWP and/or Selenium-based tests of what the application visibly does, treating it as a black-box and mapping functionality right or wrong.   This will give you an objective way of determining what your code-changes actually did to the system.

In fact, in retrospect, let me change my mind here.   I just might definitely would put the tests first of all, knowing that some of the things which cause some tests to “pass” are actually “erroneous behavior.”   Right or wrong, they are the present-state.   What I want is for the computer to objectively and automatically tell me when the behavior of the system has changed from its present-state.   Some of those changes will be for the good; others are unwanted.   In both cases, I want the computer to tell me this.

Another reason for this is that the present-state, business-supporting(!), behavior of the system is known (or presumed) to be based on errors, including source-code errors, yet the system is in production and therefore must continue to do what it does for the business, without unheralded changes.   Your tests therefore serve to express that present-state behavior so that any and all deviations from the same will be detected by the computer, which is always known to be more thorough and more detail-oriented than any of us could be.