Printer-friendly version

This rant is dedicated to code examples (found in books) that promote bad programming habits. Some of them can be counted among famous code smells.

...for God's sake, books should be educational in every aspect !

No Need For Comments

This bad example comes from Apache CXF Web Service Development book by Naveen Balani and Rajeev Hathi.

//Add Books associated with the Category
public void addBook(Category category) {
  bookMap.put(category.getCategoryId(), category.getBooks());
}

What is the comment doing there ? Ah, it is saying that the method name is not descriptive enough, so it can not be understand without additional hint. Can we get rid of this comment ? Sure we can:

public void addBooksFromCategory(Category category)
  bookMap.put(category.getCategoryId(), category.getBooks());
}

Better ? Better. See here and here for some hints on comments related code smells.

Tell Don't Ask

This bad example comes from Apache CXF Web Service Development book by Naveen Balani and Rajeev Hathi. (again !)

Take a look at this code snippet:

/**
* Validates the order and returns the order ID
**/
private String validate(Order order) {
  String custID = order.getCustomerID();
  String itemID = order.getItemID();
  int qty = order.getQty();
  double price = order.getPrice();
  if (custID != null && itemID != null
          && !custID.equals("")
          && !itemID.equals("") && qty > 0
          && price > 0.0) {
     return "ORD1234";
  }
  return null;
}

Uhm, eh... what is this method doing ? It asks other object for data and takes decision based on acquired information. A perfect example of procedural programming. ...have you ever heard about "Tell, don't ask" principle ?

For those who never heard (hey, book authors - it is for you), take a look at this code snippet:

private String getOrderId(Order order) {
  if (order.isValid())
     return "ORD1234";
  }
  return null;
}

As you can guess there is a second code smell hidden here - Order class has all possible getters and setters. I blogged some time ago about such programming style, so I won't repeat myself here.

[UPDATED 2010-01-13 This topic raised few comments so I decided to write a separate blog post to show in details what are the consequences of both design approaches.]

Test Should Be Perfect

This bad example comes from Apache Maven 2 Effective Implementation book by Brett Porter and Maria Odea Ching.

This one is different from previously presented. This is definitely not a code smell, but it stinks anyway.

The story goes like this:

• first a simple method that always return true is presented:
public boolean execute() {
  return true;
}
• then comes a simple test:
  
  @Test
public void executionSucceeds() {
  assert !action.execute();
}
• now mvn test is executed - and the test obviously fails;
• then they show that it is enough to fix the test and mvn test will pass:
  
  @Test
public void executionSucceeds() {
  assert action.execute();
}

What makes me really angry here, is that the book presents writing of shabby tests. IMHO tests are the most important - if you don't write them carefully you are in deep trouble. So, the example should go otherwise - the test should fail because of error in method body, not in the test itself. And the method should be fixed, not the test code. Howgh.
Yes, I know that the author wanted only to present mvn test feature, but I don't care.

I'll Be Back

That's it for today. I'm pretty sure I will present some more examples in the future. Stay tuned ! :)