How Programming Books Promote Code Smells

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 ! :)

Dobry post. Podobnie książki

Dobry post.
Podobnie książki do Hibernate, Springa, itp wypromowały Anemic Domain Model.

It depends on the context

If you are sure the context for a given object will always be the same, then yes, the behavior can be built in. But suppose that validation for a given chunk of data, say the core data for an invoice/order, changes with the context. What do you do then? Every time you have a new business rule that comes up where the validation for that telephone number has to validated in a different way, do you add a new method:

order.isValid() v. order.isValidForXYZ() v. order.isValidForABC()?

or order.isValid(someExternalRule)?

For each of those you are doing something that is making the code fragile and/or requires the code to be changed and/or requires you to make a subclass, all of which will have to be tested in some way.

Now, if instead you have some simple value object (DTO, whatever you want to call it), with no or minimal behavior, the object itself can be validated externally - preferably by an object implementing some interface so as business rules change and/or are added to, it is the rule that needs to be tested, not the object simply holding the data. Yes, some external object has knowledge of some of the internals of the value object (as much as you care to expose), but you don't have a value object that needs to know every single context it may be used in. In my experience, value objects like this are often used in many different contexts, or you wind up with duplicates of these objects for the different contexts, or you wind up with an object that has all this behavior internal to itself and may be very fragile and/or bloated as a result.

No one solution is golden or a big steaming pile of feces, although the fragile and bloated know it all object that can handle every context is usually to be avoided (IMO). It depends on the context, and that is where the judgement of the developer comes into play. Yes, by all means, talk about the disadvantages, but also mention the advantages and different contexts.

interesting, thank you

Hi Anonymous,

I read your comment with great interest. I think we share a very similar view, except that I believe getters/setters to be unacceptable solution in a situation like this. I'll try to explain it better in the next blog post.

--
cheers
Tomek

Consider the need for concise examples

I agree with your point that authors should demonstrate best practices as much as possible. Unfortunately, many of those best practices introduce other complexities that the author may not want to delve into, or they add so much additional complexity to the example that their original point is likely to get lost.

The best writing is concise and accurate. This is not always true of the best code: consider the amount of boilerplate code involved in a chain of responsibility pattern coupled with singleton commands, then follow the best practice of using dependency injection to configure and instantiate the chain. If all you are trying to demonstrate is how to implement an HTTP filter to ZIP and encode a little data, should you really include all of this extracurricular activity?

In both cases good code would be more concise :)

Hi Anonymous,

I get your point. In some cases sticking to good standards would require a lot of coding - that is true.
But in both cases that I presented, good code would be more concise. Especially in "Tell don't ask" you gain a lot, because OO lets you choose a proper abstraction level.

--
Tomek

Need better example and context

May be a better example in a specific context will explain this better. The example of your concise code and the original code doesn't convey the abstraction nor any good practices. This is simple example which book might be conveying, which can be understood easily, I have come across many complex code in various books which might have right abstraction, but fails to convey the point.

try/catch in main

One of my favorite examples is the main method with try/catch.

For example:
http://www.roseindia.net/java/beginners/java-read-file-line-by-line.shtml

public static void main(String args[]) {
try{
FileInputStream fstream = ...
}catch (Exception e){//Catch exception if any
System.err.println("Error: " + e.getMessage());
}
}

This try/catch is absolutely NOT NEEDED! If you skip them, the JVM does absolutely the same work. :)

Tell Don't Ask

That's not an example of Tell Don't Ask. It is Encapsulation but you are still asking what the order Id is.

oh yes it is

Hi Anonymous,

You can ask for data and do the computation yourself (give me your X, give me your Y, give me your Z and I will calculate the result) or you can tell someone to do the calculation and return the result ("order - are you valid ?").

--
Tomek

getOrderId revisited

is getOrderId an instance method of Order? (It probably should be.) Why do you need to pass it in an Order then? It looks like a static method without the static keyword.

static or not static - all I care about is OO

I concentrated only on one aspect of this method - lack of OO approach. You might be right that it should be static, but this is another topic.

--
Tomek

My point is

It shouldn't be static. It also shouldn't take an argument. It should be an instance method of Order.

.. and it should be called

.. and it should be called Order.getId(), not Order.getOrderId() if we're nitpicking..

How about comments specifically meant for javadoc?

In general, I agree that code should be self-documenting.

However, some of these examples could be destined for javadoc.

Admittedly, these examples do not strictly follow javadoc conventions. However, a bit of explanatory candy for the javadoc to munch on is better than nothing, especially if I am not necessarily looking at the code at the moment but some automatically generated documentation.

feel the pain

I feel your pain. I'm now in a group that just moved to java recently and they have fallen into the over architecture path. Namely from Rod Johnson's early book. talk about service-hell! Thrown in a bunch of procedural style programming like you mention and it is a sad state of affairs.

"Order class has all possible

"Order class has all possible getters and setters."

You are in the minority here. Object-relational tools and web developing are all base on the javabean model which means getters and setters.

getters and setters

I believe the main point was something else validating the order instead of it validating itself.

we all know getters and setters are a necessary evil if you want to use outside tools, i.e. ORMs.

"Tell, don't ask" principle

A perfect example of procedural programming. ...have you ever heard about "Tell, don't ask" principle ?

Yep, all books should be promote good practice, but that doesn't seem the case as most of books i have read seems to focus to get the point across through examples and miss out best practices:). Than you other category of specialized books which promote best pratices.
However, the correlation that you have given with the link doesn't seem to match here with this example.
The link that you mention refers to other object' state and work on it, which should be avoided.The getters are setters
are usually generated and having a validation code out there doesn't make sense to me.

Maybe there is no one and

Maybe there is no one and only true validation. Maybe different clients of the class have different validation requirements. Ever considered that?

More than one validator ?

Hi Anonymous,

please read this blog post - http://kaczanowscy.pl/tomek/2010-01/getters-setters-revisited-validation.... You will see there how to implement many validators without getters.

--
cheers
Tomek

What about http://kaczanowscy.pl/ipdl/index.html

Hi mr. Tomek,

I need that material that you post in http://kaczanowscy.pl/ipdl/index.html, but unhapply doesn't have anymore.
Please help me because I am making a job inspired on that site....!

Regards,
Rosane

please mail me

>I need that material that you post in http://kaczanowscy.pl
>/ipdl/index.html, but unhapply doesn't have anymore.
>Please help me because I am making a job inspired on that site....!

Hi Rosane,

please contact me by email:
tkaczano AT SERVER poczta.onet.pl

--
cheers
Tomek

In any case getters are not required

Hi Anonymous,

that is a good question !
If more than one validation is required, then things get more complicated. But don't worry it can be done easily without getters/setters. :)
I'll show you soon how in a separate blog post.

--
cheers
Tomek

Please comment using