Question about good coding practice

**Cornix** · August 28th, 2014, 05:27 AM

Hi there.

I have a tree-based data structure that looks somewhat like this:
image1.png
Every tree always has a height of no more then 3, and each of these classes has a very specific use, generalization is not possible between them.

Now, I want to display my data structure with a JTree in a Swing application. A JTree needs a TreeModel and so I created a custom TreeModel for my classes, it looks like this:

public class CustomTreeModel implements TreeModel {
 
	private Root root;
 
	public Object getRoot() {
		return root;
	}
 
	public Object getChild(Object parent, int index) {
		if (parent instanceof Root) {
			Root obj = (Root) parent;
			return obj.getChild(index);
		}
		if (parent instanceof Node) {
			Node obj = (Node) parent;
			return obj.getChild(index);
		}
		throw new IllegalArgumentException();
	}
 
	public int getChildCount(Object parent) {
		if (parent instanceof Root) {
			Root obj = (Root) parent;
			return obj.getChildCount();
		}
		if (parent instanceof Node) {
			Node obj = (Node) parent;
			return obj.getChildCount();
		}
		if (parent instanceof Leaf) {
			return 0;
		}
		throw new IllegalArgumentException();
	}
 
	public boolean isLeaf(Object node) {
		return node instanceof Leaf;
	}
 
	public int getIndexOfChild(Object parent, Object child) {
		if (parent instanceof Root && child instanceof Node) {
			Root p = (Root) parent;
			Node c = (Node) child;
			return p.getChildIndex(c);
		}
		if (parent instanceof Node && child instanceof Leaf) {
			Node p = (Node) parent;
			Leaf c = (Leaf) child;
			return p.getChildIndex(c);
		}
		throw new IllegalArgumentException();
	}
 
}

And I dont like it.
All these "instanceof" conditions and all the type casting is very ugly in my opinion. But this is how a TreeModel works.
So I had the idea that I might simply Introduce a new Interface, for Example the following:

public interface TreeElement {
 
	public Object getParent();
 
	public Object getChild(int index);
 
	public int getChildIndex(Object child);
 
	public int getChildCount();
 
}

But then I would introduce this interface to my Model classes just for the purpose of writing the View. So my data model has to be altered to make the View work, this sounds completely against MVC to me and also like something I dont really like.
On the other hand, the CustomTreeModel would be much simpler, easier to understand, easier to maintain and just in general more pleasing to the eye.

What are your opinions on this? Should I change my Model to improve the View? Is instanceof okay to be used in View classes?
Thank you all very much.

**aussiemcgr** · August 28th, 2014, 07:44 AM

Why isn't Root it itself a Node?

**Cornix** · August 28th, 2014, 11:02 AM

Each of these classes has very specific methods and uses. The root and the node are completely different in every aspect. The tree is always supposed to have no more then 3 elements in height.

**aussiemcgr** · August 28th, 2014, 11:25 AM

I would absolutely recommend your interface. In most cases of tree structures I've seen, the Root node is a Node, as is the Leaf. Or, if different classes for each were really needed based on implementation, Root would extend Node, and Leaf would extend Node.
Your interface idea creates a similar structure, but a bit less coupled I suppose. Including the interface would explicitly say: "Root, Node, and Leaf are all related in some way", which is what you are already implicitly saying with your current implementation.

**Cornix** · August 28th, 2014, 12:14 PM

But this is not actually a tree. I just chose to display it as a tree (for now) because a tree makes it nice to view all the data at once.
In reality this is an InformationSheet with Categories that contain Entries. The intention of it being a tree is not intended, it just happens that the data can easily be depicted as a tree.
That is why I dont really like to use such an interface because it goes against the natural intention of the data.

An InformationSheet does not have a parent. Adding a "getParent()" method is nonsense. On the other hand, Categories dont really have a parent either, they are part of an InformationSheet.
And its not like Entries have 0 children. Entries simply dont have children at all. The concept of children for an Entry within a Category is pure nonsense.
This is a design decision on the most pointless level.

**aussiemcgr** · August 28th, 2014, 01:29 PM

Ah, okay, I see. I was confused by how you had the names. Hmmm. I can think of a few things, but none of them would be nearly as neat as the interface. I'll have to think about it. Have you considered a wrapper class?

**Cornix** · August 28th, 2014, 01:44 PM

That could very well work, but it would be a lot of lines of code to work around this, but its probably the nicest and cleanest solution.

**ChristopherLowe** · August 29th, 2014, 08:32 AM

If I understand your problem correctly it looks like a good candidate for a Visitor design pattern.

public interface IElementVisitor {
    public void visit(Root root);
    public void visit(Node node);
    public void visit(Leaf leaf);
}

public interface IElement {
   public void accept(IElementVisitor visitor);
}

Now the visitable objects Root, Node and Leaf will implement IElement (hate the reference to a tree data structure here btw)

public class Root implements IElement {
 
   // The guts of the class
 
 
   public void accept(IElementVisitor visitor) {
       visitor.visit(this);
   }
}

The code is identical for the Node and Leaf classes. So the impact to your data models is a single line of code. In the CustomTreeModel you would do something like this:

public Object getChild(Object parent, int index) {
 
    IElement element = (IElement) parent;
 
    element.visit( new IElementVisitor() {
 
         @Override
         public void visit(Root root) {
                 // whatever needs doing if this is a Root
         }
 
         @Override
         public void visit(Node node) {
                // whatever needs doing if this is a Node
         }
 
         @Override
         public void visit(Leaf leaf) {
                // whatever needs doing if this is a Leaf
         }
 
    } );
 
 
    // and so on for the other methods

There will probably be a little messing around with the final modifier in this example but here is what I love about Visitors and why I think it's a good option for you:

Separation of concerns amongst a set of related classes. Your Root, Node and Leaf classes are related but need to do different things in this particular circumstance. This will defer the things that need doing to them to the classes that use them.
Syntax errors instead of run time errors when adding new classes. Lets say in a few months time you want to add a fourth level. All you need to do is add another method to the IElementVisitor and every visitation will light up red. In your posted code it will result in IllegalArgumentException that need hunting down.
Reusability. Are there any other places where the algorithm is independent of the data structure? Don't add behaviour to the data, visit it and keep the controller in charge of behaviour. This is the heart of the Open/Closed principal as it allows a set of related classes (Root, Leaf and Node) to have behaviour assigned to them without any changes to their source code.

Thread: Question about good coding practice

LinkBack

Thread Tools

Display

Question about good coding practice

Related threads:

Re: Question about good coding practice

Re: Question about good coding practice

Re: Question about good coding practice

Re: Question about good coding practice

Re: Question about good coding practice

Re: Question about good coding practice

Re: Question about good coding practice

Similar Threads

Any good websites to practice programming?

Question About Best Practice for ActionListener With Multiple Buttons

Good examples for exam practice?

Regarding good coding practices

Need help with this practice java question