Mark Crowley is a PhD student at UBC and is not TAing any courses this summer.
He is also the TA Coordinator.

Code faux Pas

During several years of teaching undergrads programming I've come across a number of common programming problems that seem to crop up. Here I present a list of some common coding faux pas with examples of improved code. Note two disclaimers:
1. Most the problems here are not actually incorrect, they usually work, but they are bad style because they are hard to read, hard to maintain or indicate a lack of stuctured planning during design
2. Programming style is very subjective, some experienced programmers may disagree with my list, I do not claim these rules are definitive, only that they are reasonable.


The needless break

The bad way:

  public boolean hasTasty(Food[] listOfFood){
     for(int i=0; i<listOfFood.length; i++){
        if (listOfFood[i].tastiness < 5)
           break;
        else 
           return true;
     }
  }
The better way:
  public boolean hasTasty(Food[] listOfFood){
     for(int i=0; i<listOfFood.length; i++){
        if (listOfFood[i].tastiness >= 5)
           return true;
     }
  }
Another way:
  public boolean hasTasty(Food[] listOfFood){
     done = false;
     answer = false;
     for(int i=0; !done || i<listOfFood.length; i++){
        if (listOfFood[i].tastiness >= 5){
           answer = true;
           done = true;
        }
     }
	  return answer;
  }
Some people would debate which of these solutions is better. I prefer the last one as it maintains a single exit point for the method. This makes it easier to analyse or even prove its correctness using loop invariants.


The excessive if

The clunky way:
  if(myList.length >= 0)
     return true;
  else
     return false;

The elegant way:
  return (myList.length >= 0)
This works because a boolean expression evaluates to either true or false.


The find and break

Often we use a loop to search for an element in some kind of collection and the question arises "What do we do when we find it?"
One standard way is to call break when the element is found This code is so compelling that sun even advises it in its tutorials. But that does not mean its a good thing:
  private static int getPos( Object[] a, Object theKey ) {
     int i;
     for(i = 0; i < a.length; i++) {
        if( a[i] != null && a[i].equals( theKey ) )
           break;
     }
     return i;
  }
This is not incorrect mind you, but consider the following alternatives: 
  for( i=0; !found && i < a.length; i++){
     if( a[ i ] != null && a[ i ].equals( theKey ) )
        found = true;
  }
  return i;
Here you there is a easy to clear logical understanding of how many times the loop will execute. Or perhaps this: 
  for( i=0; !found && i < a.length; i++){
     if( a[i] != null && a[i].equals( theKey ) )
        return i; 
  }
Which is equivalent to the break version but more concise and easier to follow.

Variables vs. return's

Sometimes we are able to return an answer as soon as we find it but need to cover cases where it is not found at all. Searching for a key in an array could lead to the following code: 

  public boolean contains(int theKey) {
     boolean contains = false;
     for (int i = 0; i < size(); i++) {
        if (keyArray[i] == theKey)
           return true;
     }
     return contains;
  }

The problem here is that the developer is mixing metaphors, they are returning literal values sometimes and variables names others. This is not neceesary, either of the following implementations would be clearer:

  public boolean contains(int theKey) {
     for (int i = 0; i < size(); i++) {
        if (keyArray[i] == theKey)
           return true;
     }
     return false;
  }
		
  public boolean contains(int theKey) {
     boolean contains = false;
     for (int i = 0; i < size() && !contains; i++) {
        if (keyArray[i] == theKey)
           contains = true;
     }
     return contains;
  }

Rule No.1 of Software Development

Thou shalt not change the design specification. Really, this is it. As a developer you should always make readable code, document it well and design for modularity and reuse. But if you don't adhere to the design spec, if you change the signatures of your methods because you don't like them, then it would have been better if you submitted an empty class with no code at all. If others cannot call your code without actually looking at your source file then its useless. They will look at the interface design and call it. What if you think Integer is better than Object for the arguments? Then talk to the architecht and get them to change it. If they won't, you have to live with it.


Return void of meaning

Ok, this one drives me crazy. Again, it works, but it adds nothing to the program except evidence that the programmer doesn't understand Java. Put simply, if your method has a void return type, then it does not return anything so don't use the return statement. No, I know, maybe it can get you out of a if or loop. Don't use it. If you must break out there is the break statement. 
  public void coolPrint(String name, double b){
    System.out.println(name + " is " + b + " \% cool!");
    return void;
  }
Just, just.... don't do this. I can't take it. return; is simply not needed at all.


Single line scoping

Block statements such as if and for do not require braces brackets if they only contqain one line of code. Java also doesn't require indentation to indicate scope, so the following is works: 
  for(int i = 0; i < 256; i++)
  myArray.add("" + (char) i);
  myArray.add("<" + 256 + ">");
  myArray.add("<" + 257 + ">");
But its very hard to read. The first add statement is in the loop and the other two are not. Why not indent it? One tab character isn't going to cost you anything. 
  for(int i = 0; i < 256; i++)
     myArray.add("" + (char) i);
  myArray.add("<" + 256 + ">");
  myArray.add("<" + 257 + ">");


Whilifing

All loops implicitly contain conditional statements so it may be tempting to use a while loop as an if: 
  while(myList.contains(shinyPenny)){
     return true;
  }
This actually works, but it denies the entire definition of what a loop is. If there is no iterator, no value being incremented multiple times then its not a loop. Don't ever do this. Use an if statement.


Hacking the Index

A for is meant to iterate in a regular way through some collection. This regular increment need not be one but its a bad idea to dynamically set the index yourself from within the loop: 
  for(int i=0; i<a.length; i++){
     if(a[i] == key){
        found++;
        i = 55;
        //or even worse...
        i = a.length;
     }
  }
A for loop continues until the boolean expression in its definition, the loop invariant, is false. Use that to stop the loop. If you need to jump ahead use a while loop so it will be expected by anyone looking at your code.


Helper Methods

A helper method is usually a short method that accomplishes one specific task that is part of a larger method. The same could be said of all methods really but some methods are higher up the chain. Ideally, no method should be more than two screens long, if it is, try abstracting out logical portions of it into seperate helper methods. You may be surprised how useful it can be. Often we repeat certain loop or conditional structures several times in a class, these repetitions should always be encapsulated into seperate methods. This helps us maintain code since we can change all the uses of the code by altering just one method.