Please find the design/implementation bugs in this code

Question

I was asked to check any design/implementation issues in this code, from a constructor/inheritance point of view. There is something about this code that does not seem right has anyone got any ideas?

Some of the points I made were one constructor was passing a null value, which in turn was setting a final variable. This variable will always hold a null value. I also mentioned that ConceptA's constructor passes in a Concept instance to be set as ConceptA's parent. This did not seem correct as ConceptA extends Concept, so could pass in a ConceptB which is concrete class of Concept and set it as the parent of ConceptA, which is not good.

First class - Concept.java

public abstract class Concept {
    private String id;

    protected Concept(String anId) {
        if (anId == null) {
            throw new NullPointerException("id must not be null");
        }

        id = anId;
    }

    public String getId() {
        return id;
    }

    public void setId(final String id) {
        id = id;
    }

    public boolean equals(Object other) {
        return other != null && other.getClass().equals(getClass())
        && id.equals(((Concept) other).id);
    }

    public String toString() {
        return "Concept(" + id + ")";
    }
}

Second class - ConceptA.java

public class ConceptC extends ConceptA {
    private static int nextSerialNo = 0;

    public static int getNextSerialNo() {
        return nextSerialNo++;
    }

    private final int serialNo;

    public ConceptC(String anId) {
        super(anId, null);

        serialNo = getNextSerialNo();
    }

    public int getSerialNo() {
        return serialNo;
    }

    public String toString() {
        return "ConceptC(" + getId() + ", " + serialNo + ")";
    }
}

Third class - ConceptB.java

import java.util.Set;
import java.util.HashSet;
import java.util.Iterator;

public class ConceptB extends ConceptA {
private final Set children;

public ConceptB(final String anId, final Concept aParent) {
        super(anId, aParent);

        children = new HashSet();
    }

    public int getCount() {
        return children.size();
    }

    public void addChild(Concept aChild) {
        children.add(aChild);
    }

    public void removeChild(Concept aChild) {
        children.remove(aChild);
    }

    public Iterator getChildren() {
        return children.iterator();
    }

    public int getFamilySize() {
        int count = children.size();

        for (Iterator iter = getChildren(); iter.hasNext();) {
            count += ((ConceptB) iter.next()).getFamilySize();
        }

        return count;
    }

    public int getAncestorCount() {
        int count = 0;
        Concept ancestor = getParent();

        while (ancestor != null) {
        count++;
        if (ancestor instanceof ConceptA) {
            ancestor = ((ConceptA) ancestor).getParent();
        } else {
            ancestor = null;
        }
    }

    return count;
    }

    public String toString() {
        return "ConceptB{" + getId() + ", parent=" + getParent()
        + ", children=" + children.size() + "}";
    }
}

Fourth Class - ConceptC.Java

public class ConceptC extends ConceptA {
    private static int nextSerialNo = 0;

    public static int getNextSerialNo() {
        return nextSerialNo++;
    }

    private final int serialNo;

    public ConceptC(String anId) {
        super(anId, null);

        serialNo = getNextSerialNo();
    }

    public int getSerialNo() {
        return serialNo;
    }

    public String toString() {
        return "ConceptC(" + getId() + ", " + serialNo + ")";
    }
}

I know this bit of problem.

public void setId(final String id) {
    id = id;
}

Could you find anything else in this code?

Answer 1

A lot of code seems to be missing (ConceptA) but here some first remarks:

In Concept:

• you overwrite equals, you should also overwrite hash code()

The Method:

public void setId(final String id) {
    id = id;
}

Should be:

public void setId(final String id) {
    this.id = id;
}

In ConceptB you take the assumption that every child is a ConceptB, this is not enforced the setter will take a Concept.

Answer 2

As was suggested, Set should have generic parameter.

    private final Set<Concept> children = new HashSet<Concept>();

    public ConceptB(final String anId, final Concept aParent) {
        children = new HashSet<Concept>();
    }

getChildren() is a very bad name for this method, because it should return children, not its iterator unless you follow JavaBeans convention (which you'd better follow). Rather than provide an access to an iterator, provide access to the whole collection:

    public Set<Concept> getChildren() {
        return children;
    }

Do not use iterators. Sometimes you might want to build some sophisticated algorithm where the order of operations is not linear; in this case iterators can be suitable. In your case it's better to use for-each loop:

    public int getFamilySize() {
        int count = children.size();
        for (Concept child : getChildren()) {
            count += child.getFamilySize();
        }
        return count;
    }