Coding without a license – ubiquitous method parameters

I saw some very poor code today, and amazingly it was written by a programmer of 10+ years.  The programmer had updated an existing class by adding a new parameter – exactly the same parameter – to each and every method of the class.  In fact, the object couldn’t work properly without this information.  Here’s my Java pseudo-code version of the updated code:

public class Before {
    public Before() {...}
    public void SomeMethod(int x, int y) {...}
    public void AnotherMethod(int x, float y) {...}
    public void AThirdMethod(int x, String a, String b) {...}
    public void YetSomeOtherMethod(int x ) {...}
}

He had also deprecated all the previously existing methods, figuring I guess that he could simply force all users of this highly shared component to update all their code.

I politely pointed out that if the data was always required, perhaps it would be better to require the data in a constructor.  Here’s  my Java version of the same change:

public class After {
    public After(int x) {
        this.x = x;
    }

    public void SomeMethod(int y) {...}
    public void AnotherMethod(float y) {...}
    public void AThirdMethod(String a, String b) {...}
    public void YetSomeOtherMethod() {...}

    // A new immutable variable, which completely
    // eliminates the pervasive x parameter
    private final int x;
}


This has a number of benefits:

  1. It simplifies the method signature of every method in the class
  2. It eliminates the need to check the parameter’s validity in every method. Instead, the parameter can be checked once by the constructor.
  3. It allows the variable to be frozen to avoid accidental tampering – in this case using Java’s “final”, but this can also be done in C#’s using “readonly”, or in VB.NET using “ReadOnly”.
  4. There is no need to deprecate all of the existing methods
  5. It may be possible to leave previous constructors in place and assume a reasonable default, or even use the C++/C#4/VB.NET feature of default parameter values to reduce rework (although this requires a re-compile).

In this case, the change was even more egregious because the class was actually implementing an interface, and the programmer had updated the interface.  Updating the interface required all implementations of the interface to pass in the “x” parameter – but “x” was an implementation detail!  So the other implementations didn’t need anything to do with “x” whatsoever!  (“x” wasn’t an integer in the real code).  Instead, using the constructor approach above, any consumer of the interface remains unchanged, and other implementations that do not rely on “x” also remain unchanged, so the impact to the API and the many, many applications that depend on the API is much less.

Think before you code, and document as you go.

Advertisements

3 comments so far

  1. David Allen on

    On the face of it, I prefer your implementation. But the key question about whether to change the interface is this: “Is the value x required for the semantics of the operations to make sense?” If so, then the painful adjustment is appropriate. If it truly is an implementation-specific change then I agree it was totally wrong to alter the interface.

    Interestingly, I find that when people routinely write contracts, the smell of these decisions are more self-evident. For example, writing a contract on the interface would raise the question “What does x have to do with the semantics of this interface?”

  2. michaeleriksson on

    That a developer of ten years makes idiotic decisions is far from unusual: The good usually become good within just a few years (while obviously not having reached mastery at that stage); the poor almost inevitable remain poor, no matter how long they stay in the business.

    A lacking understanding of the role of interfaces and contracts, in particular, is a common fault.

    (I assume that the x was intended to remain constant from call to call. If not, a closer inspection may show a lesser error of judgement.)

    • robertmccarter on

      In this case, there were only four possible values that could be passed in – and those were hard coded as constants in a different class.

      I find it disappointing that ten year “veterans” commonly make idiotic choices. From a managerial perspective these people are the experts, and management trusts what they say.

      Today I took a 200 line method written by a different veteran, and with a little refactoring and some pair-programming the two of us easily reduced it to just 60 lines of much more legible code, without a single reduction in functionality. We also removed all instances of catch blocks that swallowed System.Exception and returned an empty string.

      This allowed us to actually use the method, discover the error that was occuring, and continue working on something productive.

      Sigh.


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: