Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share

migration rejected from programmers.stackexchange.com Feb 21 at 18:38

This question came from our site for professional programmers interested in conceptual questions about software development. Votes, comments, and answers are locked due to the question being closed here, but it may be eligible for editing and reopening on the site where it originated.

closed as off-topic by Jamal Feb 21 at 18:38

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Your question must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After getting your code to work, you may edit this question seeking a review of your working code." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

4  
Hi Ali Jay and welcome to Programmers! Implementation questions are off topic, they belong to Stack Overflow, and code review questions are also off topic, they belong to Code Review Stack Exchange. I can migrate your question to either, automatically, please don't repost, but you'll have to tell us if your code is actually working or not. From what I understand you are more interested in Code Review? –  Yannis Feb 19 '12 at 16:05
1  
You haven't posted ConceptA –  Highland Mark Feb 23 '12 at 20:37
    
I can't see the logic in: public int getFamilySize() { int count = children.size(); for (Iterator iter = getChildren(); iter.hasNext();) { count += ((ConceptB) iter.next()).getFamilySize(); } return count; } Is it what you wanted? –  Highland Mark Feb 23 '12 at 20:39
    
The Set used in ConceptB isn't generic, it should be Set<Concept> –  Clockwork-Muse Dec 28 '12 at 18:25

2 Answers 2

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.

share

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;
    }
share