Review your auto-generated code

In this post I'll review several mistakes that, based on my experience, may be frequent that we make developing code in Java, and particularly using IDE's tools to auto-generate code.

Auto-generating code is fantastic, it saves us lots of keystrokes and facilitates focusing on the domain problem instead of the nuances of boilerplate code. However, sometimes we don't reflect conveniently about the code that it's generated for us by the IDE, and we leave small defects that should be fixed. In this post I'll list some of them.

Public scope

The IDEs consider all classes public by default. So, if we tell IntelliJ to create a new class, what will happen is:

public class MyClass {
}

In my opinion, classes should have package scope by default, i.e.:

class MyClass {
}

This way, they are not visible outside of the package they belong to. If, at some point, we need to change the scope of the class we will need to think why, and make the convenient design decisions. If the class is public by default, we'll use it straight away, increasing the coupling level of our code.

The best way to avoid this problem is editing the template in our IDE. In IntelliJ IDEA, for example, this option is in Preferences > Editor > File and Code template > Templates > Class.

One of the best practices when we use the package scope by default, it's creating an interface as public, but without exposing its implementations. This way clients of the interface won't have any idea about the implementation they're using, because they don't even have access to it, favoring practices like dependency injection.

As another example of this preference for public scope, IntelliJ by default creates the constants as public:

public static final String CONSTANT = "value";

Most of the times, constants are only used by the class owning them. Please pay attention to this, or change your template too.

Immutable classes

This is a detail without a direct association with auto-generated code, but that we skip 99% of the times. In general, when we want to design a class as immutable, the result will be something like this:

public class MyImmutableClass {

    private final int id;
    private final String name;

    public MyImmutableClass(int id, String name) {
        this.id = id;
        this.name = name;
    }

    public int getId() {
        return id;
    }

    public String getName() {
        return name;
    }
}

Private and final fields, no setters and initialization by constructor. OK, this is quite good, but we're missing something. Let's see what happens if we do this:

public class MyImmutableDerivedClass extends MyImmutableClass{

    public String address;

    public MyImmutableDerivedClass(int id, 
                                   String name, 
                                   String address) {
        super(id, name);
        this.address = address;
    }

    public String getAddress() {
        return address;
    }

    public void setAddress(String address) {
        this.address = address;
    }
}

An instance of this second class, which inherits from MyImmutableClass could act as a surrogate of it without clients noticing, i.e.:

MyImmutableClass myImmutableClass
                = new MyImmutableDerivedClass(1, "name", "address");

which is not ideal, because this derived class is not immutable!

The solution is simple, let's make our immutable class final:

public final class MyImmutableClass {
    //...
}

And now it won't be possible to create derived classes.

Getters and setters

It's very frequent to create a class with all its fields, and add, in an auto-generated code frenzy, its constructor, getters and setters for all the fields:

Do we really need to do this?

public class MyClass {

    private Collaborator1 collaborator1;
    private Collaborator2 collaborator2;

    public MyClass(Collaborator1 collaborator1,
                   Collaborator2 collaborator2) {
        this.collaborator1 = collaborator1;
        this.collaborator2 = collaborator2;
    }

    public Collaborator1 getCollaborator1() {
        return collaborator1;
    }

    public void setCollaborator1(Collaborator1 collaborator1) {
        this.collaborator1 = collaborator1;
    }

    public Collaborator2 getCollaborator2() {
        return collaborator2;
    }

    public void setCollaborator2(Collaborator2 collaborator2) {
        this.collaborator2 = collaborator2;
    }
}

A class should expose the minimum number of internal details. So, having said in the first section that a class should have package scope until it requires to be public, now I say that a class shouldn't have getters or setters until it's needed. And when it happens, think if that's the best decision or if there's something in your design that is not appropriate.

Equals method

In general, IDEs expose an option to generate the methods equals and hashCode together. This makes sense, because we usually need a right and consistent implementation for both methods (I won't get into many details here, assuming we all know the equals and hashCode contract).

I don't have many objections to the implementation that IDEs generate for hashCode, they're usually quite optimal, even though maybe we could optimize them even more. There's a small problem with the equals implementation though:

public class MyClass {

    private final int id;
    private final String name;

    //Constructor, equals, etc

    @Override
    public boolean equals(Object o) {
        if (this == o)
            return true;

        if (o == null ||
            getClass() != o.getClass())
                return false;

        MyClass myClass = (MyClass) o;

        if (id != myClass.id)
            return false;

        if (name != null ?
            !name.equals(myClass.name) :
            myClass.name != null)
                return false;

        return true;
    }
}    

Everything seems correct here, but something serious is happening, this class is violating the Liskov Substitution Principle. This principle is part of the SOLID principles, and in few words it says that "a derived class can act as its base class without clients noticing it". This is a bit rough definition (this principle would require an entire post), so I think I'll illustrate the problem better with an example, so that we can see why our equals method is broken.

Let's create a derived class:

public class MyDerivedClass extends MyClass {
    private final String address;

    public MyDerivedClass(int id, String name, String address) {
        super(id, name);
        this.address = address;
    }

    public String getAddress() {
        return address;
    }

    // equals and hashCode
}

I omit equals and hashCode implementations, as they're not necessary for the example.

public static void main(String[] args) {
    MyClass object1 = new MyClass(1, "name");
    MyDerivedClass object2 = new MyDerivedClass(1, "name", "address");

    System.out.println(areEquals(object1, object2));
}

public static boolean areEquals(MyClass object1, MyClass object2) {
    return object1.equals(object2);
}

The method areEquals receives two instances of MyClass. Both instances are identical based on the information they contain, so we should expect areEquals to return true. But this is not the case, because the auto-generated equals method is doing this:

if (o == null ||
    getClass() != o.getClass())
        return false;

getClass returns MyClass for the first object and MyDerivedClass for the second, so our method equals returns false.

We could discuss about the conveniency of this to happen, because both are instances of different classes. But, does it really make sense when the values for all the fields in both instances are exactly the same? In fact, one alternative implementation of areEquals could be:

public static boolean areEquals(MyClass object1, MyClass object2) {
    return object1.getId() == object2.getId() &&
            object1.getName().equals(object2.getName());
}

In this case, the result is true.

This is what Liskov Substitution principle is about. One of the implications of this principle is that, if we overwrite a method it will be to add some behaviour without removing anything the base class is doing. Something that breaks this principle dramatically is the practice of overwriting a method to leave it empty (I guess many of us have done this at some point in our careers).

So, we need a better implementation for our method equals. And it would be something like this:

@Override
public boolean equals(Object o) {
    if (this == o)
        return true;

    if (!(o instanceof MyClass))
        return false;

    MyClass myClass = (MyClass) o;

    if (id != myClass.id)
        return false;

    if (name != null ?
            !name.equals(myClass.name) :
            myClass.name != null)
                return false;

    return true;
}

Instead of interrogating the objects to know their class using getClass, what we do is asking the object passed by parameter to equals if it's an instance of the class where equals lives through the operator instanceof. This operator returns true for instances of that class and its derived classes too. And now, finally, our areEquals method works the way we want. All this (and more) is explained perfectly in the seminal book Effective Java, book that every Java developer should have on his library.

The truth is that IntelliJ exposes a way to auto-generate this version of equals, but we must be careful with the options of the wizard, because it's unchecked by default, and we must check it ourselves. It is "Accept subclasses as parameters to equals() method":

The dialog box itself mentions that the implementation by default breaks the equals contract. It seems the instanceof implementation may not be compatible with some frameworks. In that case, we should switch to the first version, but always being aware of the consequences. So, let's use the correct version in the first place, and let's switch to plan B if necessary :)