Thoughts on using subroutines to clean up code?

[nysus]

Looking for some the thoughts and opinions from my esteemed and seasoned Monks on using subroutines to make code more readable/maintainable/testable. Inevitably, I find my code starts out nice and clean but then edge cases start cropping up that were hard to anticipate and I have to pepper my subroutines with additional checks and statements to account for these edge cases which end up turning my subroutines into a mess. So I find myself very tempted to dump out some of these edge cases to subroutines even if they will probably never be used by another bit of code. Speed is generally not an important factor for my purposes.

I'm curious to know what others think about doing this and what problems it might create even if the practice is used judiciously.

[Laurent_R]

I agree with what has been said previously by other monks. Just a couple of additional thoughts.

dump out some of these edge cases to subroutines even if they will probably never be used by another bit of code

Doesn't matter. Yes, subroutines are one way to reuse code, but they serve other purposes too. For example, you can also pack edge cases into subroutines if this makes it possible to make the main flow of the program easier to follow. Or the main loop of your algorithm shorter. Don't worry if some of your subroutines get called in only one place, there is nothing wrong with that.

Additional advice: your subroutines should usually have no side effect and should ideally not rely on global variables. If possible, they should be able to work alone with the parameters they get, and they should return their result. There are tenets of the functional programming paradigm, you don't have to always adhere to them fanatically (sometimes you need to use an environment variable or it make sense to use a global hash or array), but if your subroutines are functional in the sense that they rely only on input parameters and return to the caller output values, then your life will probably be easier. If only because it will be easier to test them out of context. So make your subroutines functional when possible.

On the other hand, there are also some trade-offs. If you have a very large number of very small subroutines, it can also become distracting and make your program harder to follow. I have worked long ago on C++ code where almost every single line of code was a method invocation, and when you where looking at the method code, it was just another series of method invocations, and so on. This might tend to become "write-only" code: relatively easy to write, but very difficult to understand or to debug if you have to follow a dozen of nested method invocations to finally find out which piece of data is really being used or updated. So, I would say: use your judgment and good sense, it make sense to break your code into smaller subroutines (or methods), but don't overdo it either, it might become counterproductive. In short, your subroutines should not be too long, but it might also be wrong if most of them are just one or two lines long.

Well, my two-cents.

[salva]

I have worked long ago on C++ code where almost every single line of code was a method invocation, and when you where looking at the method code, it was just another series of method invocations, and so on.

There is a name for that: Ravioli code!

[nysus]

Thanks, this helps. For some reason, I have it my head that I should only create a subroutine if it will be used more than once. Knowing that that isn't a hard and fast rule helps.

[stevieb]

Often, I put parameter and other validation routines into 'private' subs (leading underscore on a sub name is not enforced, but it's widely known not to use these special functions directly). They are often fit for a single purpose, and not reused. Here's an extremely trivial example. Now, if there's a bug found where we need another check, it goes into the validation function, and doesn't pollute the main subroutine:

use warnings;
use strict;

sub func {
    my ($x, $y) = @_;

    _validate($x, $y);
    print "$x, $y\n";
}
sub _validate {
    my ($x, $y) = @_;

    die if $x <= 20;
    die if $y !~ /^\w\d{2}/;
    die if length $y > 10;
    die if $y =~ /y/;
}

This is especially handy when using an editor that folds your subs for you...

[Your Mother]

It's not a rule, just a rule of thumb: if a subroutine is too long to fit in a terminal screen, say 50x70, the subroutine is probably too long.

What was said already applies. With unit tests (a test for each sub), refactoring is safe and often fun. So write them if you don't already have them. Without examples, concrete advice that won't be misconstrued is hard to offer.

[stevieb]

An example or two would be very helpful here. This is an opinion-type question, and there are going to be varying opinions on how someone would approach this depending on what the actual situation is.

Subroutines should perform a specific task, so if the edge case issue is related to one of those specific pieces of functionality, add the checks in the appropriate sub. It helps if you can keep your subs short enough to not extend past a full screenful of text.

[salva]

IMO, the key point when deciding to convert some piece of code into a helper subroutine is considering if there is a conceptual boundary around it. Can you put a name on that? are the input arguments and the outputs evident?

Another good question you could ask yourself is: when I come back to this code in a couple of months would I be able to understand the main subroutine just looking at it, without having to inspect the helper too?

[Anonymous Monk]

This is called refactoring and if you have good quality unit tests in place that provide ample coverage then you will be able to confidently refactor your code to your heart's content without introducing new bugs.

[GotToBTru]

Follow the advice here, and you will be well equipped to move beyond subroutines and into modules and/or objects.