CodeSOD: What a More And

Today, we’re going to start with the comment before the method.

    /**
     * The topology type of primitives to render. (optional)<br>
     * Default: 4<br>
     * Valid values: [0, 1, 2, 3, 4, 5, 6]
     *
     * @param mode The mode to set
     * @throws IllegalArgumentException If the given value does not meet
     * the given constraints
     *
     */

This comes from Krzysztof. As much as I dislike these JavaDoc style comments (they mostly repeat information I can get from the signature!), this one is promising. It tells me the range of values, and what happens when I exceed that range, what the default is, and it tells me that the value is optional.

In short, from the comment alone I have a good picture of what the implementation looks like.

With some caveats, mind you- because that’s a set of magic numbers in there. No constants, no enum, just magic numbers. That’s worrying.

Let’s look at the implementation.

    public void setMode(Integer mode) {
        if (mode == null) {
            this.mode = mode;
            return ;
        }
        if (((((((mode!= 0)&&(mode!= 1))&&(mode!= 2))&&(mode!= 3))&&(mode!= 4))&&(mode!= 5))&&(mode!= 6)) {
            throw new IllegalArgumentException((("Invalid value for mode: "+ mode)+ ", valid: [0, 1, 2, 3, 4, 5, 6]"));
        }
        this.mode = mode;
    }

This code isn’t terrible. But there are all sorts of small details which flummox me.

Now, again, I want to stress, had they used enums this method would be much simpler. But fine, maybe they had a good reason for not doing that. Let’s set that aside.

The obvious ugly moment here is that if condition. Did they not understand that and is a commutative operation? Or did they come to Java from LISP and miss their parentheses?

Then, of course, there’s the first if statement- the null check. Honestly, we could have just put that into the chain of the if condition below, and the behavior would have been the same, or they could have just used an Optional type, which is arguably the “right” option here. But now we’re drifting into the same space as enums- if only they’d used the core language features, this would be simpler.

Let’s focus, instead, on one last odd choice: how they use whitespace. mode!= 0. This, more than anything, makes me think they are coming to Java from some other language. Something that uses glyphs in unusual ways, because why else would the operator only get one space on one side of it? Which also makes me think the null check was written by someone else- because they’re inconsistent with it there.

So no, this code isn’t terrible, but it does make me wonder a little bit about how it came to be.

Source: Read More