Wednesday, October 3, 2012

Variables considered harmful

When I look through other code written by other developers, I'm often shocked at how often I find variables used when there is no reason for them to be variable.

Let's take a look at this awful specimen:

public class VariablesConsiderHarmful {

 public static void main(String[] args) {
  new VariablesConsiderHarmful().doYourStuff(args);
 }

 /**
  * @param args
  */
 private void doYourStuff(String[] args) {
  // Do some initialization
  
  String x = null;
  String y = null;
  if ("x".equals(args[0]))
   x = args[1];
  if ("y".equals(args[0]))
   y = args[1];
  if (x == null)
   x = "xxx";
  if (y == null)
   y = "yyy";
  
  System.out.println("x=" + x + ", y=" + y);
 }
}

Let's suppose that the middle section of doYourStuff  (between // do some initialization and the System.out.println line) is much longer and more complex than I've suggested here.  Naturally, you would like to refactor it into a private method getXY().  But if you do that, you will find it is impossible due to there being two returned values (x and y).

How about refactoring into two private methods: getX() and getY()?  The code is so convoluted (believe me, I've seen much worse) that it is very difficult to separate the definition of x from the definition of y.  That's largely because of the way they have been expressed, quite unnecessarily, as variables.  And yet, there never was any reason to make them variables. I believe this idea of declaring all of your variables at once and later filling in their values is a relic of the bad old days of programming in languages like FORTRAN IV where everything was a variable (even things that you think are constants).  But we going back 30+ years now.  Why would anyone program this way in Java today?  Yet they do!

Apart from obfuscation, there is a serious danger in this manner of coding: it's easy to miss initializing something because of a logic flaw.  If you have the appropriate Java warning turned on, it will warn you in the case where you initialized the variable with null (implicitly or explicitly).  But I've seen cases where the variable was initialized with an empty String even though that empty value would be invalid.  The compiler can't protect you then from your own folly.

Let's clean up the doYourStuff method somewhat:

 private void doYourStuff(String[] args) {
  // Do some initialization

  final String x = "x".equals(args[0]) ? args[1] : "xxx";
  final String y = "y".equals(args[0]) ? args[1] : "yyy";

  System.out.println("x=" + x + ", y=" + y);
 }

Isn't that so much more elegant?  If you want to, we can easily create a getX() and getY() method using our "Extract Method" refactoring twice.

Yes, of course, some of you will be thinking that we should be using Scala or some other functional programming language where variables are the exception rather than the norm.  I totally agree.  But let's try to avoid using variables when variability serves no purpose whatsoever!

OK, back to work!

No comments:

Post a Comment