Design of Concept Class

Question

I need to find the flaws in this design. So far the only ones i can think of are:

• No use of generics
• the class Concept uses a parametrized constructor, which means every sub class would need to pass a parameter. The setter method could be used instead.

Can you think of anymore flaws?

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 ) //changed
  {
    this.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 + ")";
  }
}

ConceptA.java

public class ConceptA extends Concept
{
  private final Concept parent;

  public ConceptA( String anId, Concept aParent )
  {
    super( anId );

    parent = aParent;
  }

  public Concept getParent()
  {
    return parent;
  }

  public String toString()
  {
    return "ConceptA{" + getId() + ", parent=" + parent + '}';
  }
}

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() + "}";
  }
}

ConceptC.java

        package com.result.exam.a;

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 + ")";
  }
}

Answer 1

i would rewrite your equals method a bit (just for readability)

public boolean equals(Object other) {
    if(this == other) {
        return true;
    }

    if(!(other instanceof Concept)) {
        return false;        
    }

    Concept otherConcept = (Concept) other;

    return id.equals(otherConcept.id);
  }

And concerning your parameterized constructor I don't see anything the speaks against this. It is part of the contract of you model and if you don't intend to use this class in any framework that requires a parameterless constructor to work i think we are fine

Answer 2

The casting in getFamilySize() is very dangerous. Notice that

public void addChild( Concept aChild )

means you can add any sub-class of Concept as a child, and therefore getChildren() returns an iterator to any sub-class of Concept.

Furthermore, you're only finding the size two levels down (the current Concept's children and their children). I suspect you want to find the size of the entire sub-tree.

A safer and more correct way to implement the method would be:

public Set getChildren()
{
  return Collections.unmodifiableSet(children);
}

public int getFamilySize()
{
  int count = 0;

  Queue<Concept> queue = new LinkedList<Concept>();

  queue.addAll(this.getChildren());

  while (!queue.empty())
  {
    Concept concept = queue.poll();
    // count the Concept we just pulled off the queue
    count++;

   if (concept instanceof ConceptB)
   {
     // we need his children too
     queue.addAll(((ConceptB) concept).getChildren());
    }
  }

  return count;
}

Answer 3

Others have pointed out some potential problem with it, but I'll try to compile them and add my own.

Concept

• Readability of equal method (Marco)
• Not checking for null in the setId method

ConceptB

• I would rename getCount to getNumberOfChildren or getChildrenCount just to make it more readable

• Question you want to consider is what kind of children can ConceptB have? If it's only ConceptB, you may want to consider addChild only takes in ConceptB and specifying which type of data the set hold to avoid casting. And as kuporific said, downcasting without checking is dangerous. I would rewrite it something like this:

  public int getFamilySize() {
  int count = children.size();
  
  for (Iterator iter = getChildren(); iter.hasNext(); ) {
      Concept child = (Concept) iter.next();
      if (child instanceof ConceptB){
          count += ((ConceptB) child).getFamilySize();
      }
  }
  
  return count;
  

  }

ConceptC

• Since ConceptC doesn't use a parent and the only thing that ConceptA adds to Concept is a parent, it might be better for ConceptC to extend Concept instead of ConceptA.

Last thing, for all subclass of Concept, there is no equal method. So for example, if I have two classes with the same id, but different parent, they are still equal. This may or may not be what you want. But I think it's something to take note of.

Concept parent = new ConceptC("parent c");      
Concept concept1 = new ConceptA("a", null);
Concept concept2 = new ConceptA("a", parent);

System.out.println(concept1.equals(concept2));