3

Recently when performing a code review I came across something like this:

Old Code:

 WriteLabour(useNewVersion);
 WritePart();
 WriteNonLabour(useNewVersion);

New Code:

WriteLabour();
WritePart(useNewVersion);
WriteNonLabour();

Now let me give you some background. This is very old code, not very object-oriented. There are always two versions of the file, the current version and the previous version. The useNewVersion flag determines if the current or previous version is to be written. The file is for a customer to consume and when they come up with a new version, they send us the new spec, we change the code to output the file as per new spec and this becomes the new version. What used to be the new version now becomes the previous version, ability to write version even previous to that is thrown away forever (that code is deleted).

The programmer was updating the code for the new spec just received. He found after deleting code for the version to be forgotten that WriteLabour() and WriteNonLabour() no longer had any version specific code, while WriteParts() now had to have version specific code. So he changed the method signatures as I have indicated above.

My suggestion in the code review was that it would be nice to keep method signatures consistent and always supply the useNewVersion flag, even if the code within the method did not use the flag. To me this made intuitive sense, because it is very likely that future versions of the file will require the flag to be passed in again and the interface for the caller would remain consistent. His argument against my suggestion was that passing the useNewVersion flag to methods that did not need it was design bloat. Who knows what other parameters the methods might need, should we pass them all in anticipation?

My gut feeling was that his argument was flawed in this particular case, but I could not counter it effectively. My question then is if there are design decisions we know are coming or by past experience, is it wrong to incorporate them into the current design even though they may be presently unused? Specifically in terms of public interfaces, method signatures etc, not private code.

But keeping code lean and not over designing is important too. Don't want to be the architecture astronaut.

Polite note: Please don't derail the topic by suggesting re-factorings for the code. This is not supposed to be a critique of the design of our legacy code. That code isn't up to standards, and I know that.

  • 1
    Would it be an error to call one function with useNewVersion true and another with it false? If so, you might be better off having a file-scope or class-scope constant that indicates which you should use, so as to avoid the risk of calling the functions with inconsistent parameters. – Ray Oct 9 '15 at 17:49
  • @Ray Yes this would cause an error. useNewVersion is currently read from the database and fed to the functions, so the user can switch it on/off. As I said not the best design, but we seldom visit these dark and dank places in our codebase, and every programmer who goes there want to be out asap. When the customer moves on one day this code will be forever deleted. One day! – Ali Oct 9 '15 at 21:47
  • Iff you actually need to keep the old way and the new way to do things, sometimes namespacing and judicious importing can help simplify everything. – Deduplicator Oct 10 '15 at 7:46
7

I think what you're dealing with is a collision of a few principles.

On the one hand, you have the principle of least astonishment which would encourage you to keep your method signatures consistent.

On the other hand, YAGNI (you aren't going to need it) tells you to leave it out as it's not necessary.

And those principles have to be weighed against refactoring costs.

It sounds like this legacy code has beaten you into a particular approach for dealing with it. You've become used to having to specify useNewVersion and numb to the information leakage from that particular design. To be a bit unfair, what you suggested to your co-worker was "I'm used to being bludgeoned over the head and having to figure out which version I should be calling. Let's not change our ways because I'm used to being hit."

And your co-workers response was to the effect of "Look, see, we don't have to get smacked all the time anymore!" Regrettably, he re-implemented that same bludgeoning approach with a different method so he doesn't get full credit in this case.


What should you do? I don't believe you should be specifying useNewVersion as a function parameter. It's a poor design and it forces the developer to be thinking about things he shouldn't have to worry about. I believe in treating functions like they are contracts. "If you call this method, it will do XYZ. You don't have to care about how it does XYZ, it will take care of that for you."

So WriteLabour() and WriteNonLabour() shouldn't have useNewVersion parameters.

  • I accept this answer because I think @GlenH7 has best understood my conundrum and explained it back to me very well. Thank you. – Ali Oct 9 '15 at 21:45
  • You have correctly pointed out I value consistency, but in this instance I took my colleagues point and redacted my review comment. – Ali Oct 9 '15 at 21:54
8

"Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away."

Antoine de Saint-Exupery

If you clutter code with unused parameters, other people reading it later will waste time trying to work out why they're there. Did you mean to put them there? Forget to remove them? Is there a bug due to missing code? They create noise that hides the real information in the code. This makes that code more difficult to read and makes people more nervous of breaking things when they change it.

Further, many modern compilers will flag unused parameters and variables as warnings. Good practice says that all warnings should be treated as errors. In this context, unused parameters are a compiler error and therefore need removing.

TL;DR: don't put unused parameters into your code "just in case I need them later".

  • Fair point, but in this instance the parameter is not a "just in case", it will be needed as it is a deciding factor of what is output and the public interface will have to keep changing. I was stressing consistency over brevity. – Ali Oct 9 '15 at 21:42
5

I hope this answer doesn't fall into the “derail the topic by suggesting re-factorings for the code” category. Nevertheless, I'd like to suggest that you replace the boolean useNewVersion flag with an integral version parameter that all functions should accept. The implementation of the function can then make decisions like this.

final int newVersion = 424;
final int oldVersion = 423;
if (version == newVersion) {
    // Do what is appropriate for the “new” version
} else if (version == oldVersion) {
    // Do what is appropriate for the “old” version
} else {
    throw new VersionNotSupportedException(version);
}

If the action to be taken for the two currently supported versions happens to be identical, the function can still make good use of the parameter.

final int newVersion = 424;
final int oldVersion = 423;
if (version != newVersion && version != oldVersion) {
    throw new VersionNotSupportedException(version);
}
// Do what is appropriate for both versions

The problem with the useNewVersion flag is that “new” doesn't really have any meaning and therefore, your colleague is not completely wrong saying that there is no way he could act upon it sensibly so the parameter shouldn't be passed in the first place. On the other hand, stable interfaces are a good thing to have. I'd also consider it desirable to have all functions follow a common convention, such as taking the version as their first parameter. It can help reduce confusion when looking at a function call. If you can already anticipate that you'll need the parameter again in a future version, repetitively adding and removing it is time not spent very effectively. Especially if you propagate the version information through several layers of function calls.

You've said that the useNewVersion flag is loaded from some user configuration. That's fine and you don't have to change it. Just alter your internal code to translate that boolean flag into an integer. You can have one central function that does this.

int getCurrentVersion() {
    final int newVersion = 424;
    final int oldVersion = 423;
    final boolean useNewVersion = loadFromDatabase("useNewVersion");
    return useNewVersion ? newVersion : oldVersion;
}

Using an enumeration instead of an integer could also be an option.

This seems like a very moderate refactoring effort to me that would help make your code more robust and consistent. If you now receive a new specification from your customer and forget to update some function, you will be told immediately because the new version number is not recognized by them. With the boolean flag, you would instead have silently reversed the meaning of “old” and “new” and a function you forgot to update would have happily done the wrong thing.

  • 1
    Thank you, your answer makes an important case for consistency when you say, "if you propagate the version information through several layers of function calls". This was a simplistic case with only one layer. Your suggestion to use a version parameter is also valid and identical to what @GlenH7 suggested, "treating functions like they are contracts", in this case always taking a version. I also realised the useNewVersion is so named because the programmer who originally wrote the code named it after the GUI tick box presented to the user to enable the new version spec. – Ali Oct 10 '15 at 7:52
1

... if there are design decisions we know are coming or by past experience, is it wrong to incorporate them into the current design even though they may be presently unused?

Let's bypass the level of predictability of whether or not you'll need the feature. That's a risk you'll just be taking. If we assume the features are going to get added,the question is when are they the easiest to add, now or later. If they're much easier to add now, you may want to reconsider the design, but you indicated that's not the issue.

Now all you have to go by is do you have time now or will you have time later? It sounds like you're anticipating a lot of new features at some point and we all know they'll want them sooner rather than later. Choosing to add them now when you have the time, could mitigate the risk of whether or not you need them at all.

I think is is always easier to add things later, compared to trying to identify unused/unneeded code later on and having to figure-out how to get rid of it. Either have good notes and/or memory. I just don't think it is worth it.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.