Bad sample code…

Okay, I just HATE bad code.

I’m not religious about how you express your business logic in the code. I don’t mind “different” indentation or long methods although I try to keep my own code as short and simple as possible. I prefer lots of small classes over a few huge, and I prefer a bunch of short methods (“a screenfull”) over a few huge ones. Complex IF-clauses formed as simple methods etc.

Whenever I’m maintaining code developed by others, I accept the previous developers different habits. I might rewrite (also known as refactoring) parts, especially if there are unit-tests. But only if I think it would benefit the quality and maintainability of the code.

But, when reading books or articles I get really REALLY annoyed when I encounter bad code and bad habits in samples etc. Whenever I come across these things, where the quality is as if the developer is not really a JAVA developer, absentminded or just plain drunk, I loose interest in the rest of the content. As if, if the code is really bad (and untrustworthy) how can I then trust the rest of the content?

Oracle

Let me give an example. I was reading up on the latest stuff in JEE 7. The JEE tutorials are not the best but does give you a good tour about the new features and such. As a seasoned Enterprise JAVA developer I’m mostly interested in the new features. Just enough information for me to know what to be able to take advantage of the next time I’m on a “new” JAVA EE project.

So, here I was, reading about the Facelets, which should be the new black. I’ve always hated JSF for being over-engineered and way too complex for most applications. On the other hand, there are IMHO no really good (as in KISS, no crap – just let me code that HTML/CSS/js/Ajax etc) JAVA web frameworks (sorry, but Play doesn’t count, it’s crap too in its own way) so facelets just might be the right framework for my future work.

So here I was reading about the “guessnumber-jsf” example application and stumbled upon the UserNumberBean implementation and started to feel sick…

Now… JAVA and JEE are (currently) owned by Oracle and you should think they would spend some time and money writing proper documentation, as this is actually MARKETING for their products. But for whatever reason, Oracle has decided to save money on the “samples code” part and has chosen a junior developer from one of the grammar-schools in the neighbourhood to do the coding (which might be okay) and has chosen NOT to have someone with just the slightest experience in proper JAVA coding doing any kind of peer-review. The result is, well, questionable…

Hey, I know, this is just some stupid sample code doing nothing terribly important, but not spending just a few minutes on the implementation annoys me. Why not, why not just review your own coding and think (as in “Think McFly“)??!

Let me tear it apart:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57

package javaeetutorial.guessnumber;
 
import java.io.Serializable;
import java.util.Random;
import javax.enterprise.context.SessionScoped;
import javax.inject.Named; 
 
@Named
@SessionScoped
public class UserNumberBean implements Serializable {
 
    private static final long serialVersionUID = 5443351151396868724L;
    Integer randomInt = null;
    Integer userNumber = null;
    String response = null;
    private int maximum=10;
    private int minimum=0;
 
    public UserNumberBean() {
        Random randomGR = new Random();
        randomInt = new Integer(randomGR.nextInt(maximum + 1));
        // Print number to server log
        System.out.println("Duke's number: " + randomInt);
    }
 
    public void setUserNumber(Integer user_number) {
        userNumber = user_number;
    }
 
    public Integer getUserNumber() {
        return userNumber;
    }
 
    public String getResponse() {
        if ((userNumber == null) || (userNumber.compareTo(randomInt) != 0)) {
            return "Sorry, " + userNumber + " is incorrect.";
        } else {
            return "Yay! You got it!";
        }
    }
 
    public int getMaximum() {
        return (this.maximum);
    }
 
    public void setMaximum(int maximum) {
        this.maximum = maximum;
    }
 
    public int getMinimum() {
        return (this.minimum);
    }
 
    public void setMinimum(int minimum) {
        this.minimum = minimum;
    }
}

Line 13, randomInt: Why is this not “private” and why is it an Integer. It is immediately – in the constructor – initialized to some random value and never changed. There is no need to be able to handle a “null” value. This, IMHO, should be “private final int”.

Line 14, userNumber: Well, I’m perhaps just pedantic, but it should be private; there are getters/setters and no need for anyone else to be able to access this field directly.

Line 15, response: Nice String, and there is a getResponse() method in the code. But the response field is never accessed. Never read nor written. So, why is it there? Perhaps the developer had a nice idea about it and then disbanded the idea, forgetting everything about this field. Then remove it stupid! (should be private btw)

Line 16..17, maximum and minimum: Nice idea, we should be able to configure our application to be able to deal with other cases than 0..10. But! The randomInt is initialized in the constructor with the values given, so even if there are getters/setters for these fields, there is no way we’re ever going to be able to support anything but 0..10… Remove them or refactor the code to be able to handle different maximum/minimums!

Line 20+, the constructor: Okay, why use a new Random instance every time? It could be “static final”. And why not take advantage of the new Java 7 ThreadLocalRandom?
The Integer randomInt is initialized: A “new Integer” is created. I’d say you should always rely on the “built-in” auto-boxing features, or at least use the Integer factory methods (valueOf()). But anyway, the randomInt should be an “int” and then there is no need for boxing or factory methods or new instances…
Another issue is that this implementation is difficult to unit-test. We could have a different “test only” constructor where the randomInt is given, or provide a simple “random integer provider” implementation.
Then there is “System.out.println”… This is so stupid whoever did this should be fired! Repeat after me: Don’t ever do “System.out.println” in EA code. Don’t ever do “System.out.println” in EA code. Don’t ever do “System.out.println” in EA code. Don’t ever do “System.out.println” in EA code… Not even in “your own private debug code”! There is NO excuse!

Line 35+, getResponse: Okay… There are several different ways of comparing integers for equality. One is just “==”, another is using “equals()”. Then of course there is “compareTo()”. It does work, it is not wrong to do it. But if you want to compare two items for equality, you do it by using “==” or “equals()” (depending on context) and by doing that, you demonstrate your intentions that you are interested in equality. Using compareTo (except when dealing with BigDecimals) means you want to know which number is bigger than the other, being equal is just a corner-case…
Then there is the “String” response. Whenever you are doing UI development, you should consider i18n. Returning hardcoded strings in whatever native language is yours, is basically a bad idea. As a Dane this is obvious to me…

I’m not going into details about the related xhtml facelets page, but it is equally stupid. In the “question” and “validate” parts it reads minimum/maximum from the session-scoped bean; in the title (h:inputText) it specifically refers to 0 and 10. Do or DON’T, there is no TRY…

Here’s how I would do it (before refactoring the xhtml):

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60

package javaeetutorial.guessnumber;
 
import java.io.Serializable;
import java.util.concurrent.ThreadLocalRandom;
import java.util.logging.Level;
import java.util.logging.Logger;
 
import javax.enterprise.context.SessionScoped;
import javax.inject.Named;
 
@Named
@SessionScoped
public class UserNumberBean implements Serializable {
	private static final long serialVersionUID = 5443351151396868724L;
	private static final Logger log = Logger.getLogger(UserNumberBean.class.getName());
	public static final int MINIMUM = 0;
	public static final int MAXIMUM = 10;
 
	private final int randomInt;
	private Integer userNumber = null;
 
	public UserNumberBean() {
		this(ThreadLocalRandom.current().nextInt(MAXIMUM-MINIMUM+1)+MINIMUM);
	}
 
	UserNumberBean(int randomInt) {
		this.randomInt = randomInt;
		// Print number to server log
		log.log(Level.CONFIG, "Duke's number: {0}", randomInt);
	}
 
	public void setUserNumber(Integer userNumber) {
		this.userNumber = userNumber;
	}
 
	public Integer getUserNumber() {
		return userNumber;
	}
 
	public boolean isRightNumber() {
		return this.userNumber != null && this.userNumber == randomInt;
	}
 
	public String getResponse() {
		// TODO: i18n...
		if (!isRightNumber()) {
			return "Sorry, " + userNumber + " is incorrect.";
		} else {
			return "Yay! You got it!";
		}
	}
 
	public int getMaximum() {
		return MAXIMUM;
	}
 
	public int getMinimum() {
		return MINIMUM;
	}
}