Verify code correctness

[DarwinNE]

Hi to all,
I will refer here the places where I have some doubt concerning the code.
Please feel free to comment.

cheers,
D.

[DarwinNE]

In class librarymodel/LibraryModel.java:

https://github.com/DarwinNE/FidoCadJ/blob/master/src/net/sourceforge/fidocadj/librarymodel/LibraryModel.java

Some doubts exist here:

    // NOTE: This implementation is incorrect. (DB why? equals may fail?)
    private Object getParentNode(Object node)
    {
        for(Library l:getAllLibraries()) {
            if(node.equals(l)) {
                return null;        // A library does not have any parent!
            }
            for(Category c:l.getAllCategories()) {
                if(node.equals(c)) {
                    return l;
                }
                for(MacroDesc m:c.getAllMacros()) {
                    if(node.equals(m)) {
                        return c;
                    }
                }
            }
        }
        return null;    // Node not found
    }

How can the code be improved in your opinion?

[wruggeri]

Unfortunately, I have got no time to read the whole source code, so my suggestions may be impractical or unfeasible... having said this, I'd suggest to check what the object is before performing unnecessary cycles: if the object is a category, for example, there's no reason to scan all the macros: it's enough to scan the categories, i.e. the software needs to work with two cycles instead of three, which may be a substantial performance improvement.
That's, roughly, how I would suggest to proceed:

// NOTE: This implementation is incorrect. (DB why? equals may fail?)
    private Object getParentNode(Object node)
    {
        for(Library l:getAllLibraries()) {
            if(node.equals(l)) {
                return null;        // A library does not have any parent!
            }
            if (!(node instanceof Library)) {
                for(Category c:l.getAllCategories()) {
                     if(node.equals(c)) {
                         return l;
                     }
                     if (!(node instanceof Category)) {
                         for(MacroDesc m:c.getAllMacros()) {
                             if(node.equals(m)) {
                                 return c;
                             }
                         }
                     }
                }
            }
        }
        return null;    // Node not found
   }

[DarwinNE]

That's a very interesting suggestion, @wruggeri!
I will have a more detailed look, but for the moment it seems to me that your suggestion is relevant.

Cheers,
D.

[DarwinNE]

Dear @wruggeri
I had a more detailed look at what you suggest.
getParentNode is a private method used several time in the LibraryModel class.
Most of the times, it is called with objects that are instances of MacroDesc or Category class and never with instances of Library.
If you want to modify the code by yourself and open a pull request, feel free to do that, it should make the code more robust, indeed.

However, I think that the behavior will not change very much in a practical way given how this function is used in the class.

The point is that I do not remember exactly who wrote that comment // NOTE: This implementation is incorrect., I think Kohta Ozaki did (back when the project was hosted on SourceForge), but I studied the code once more and I am not able to spot any potential error.

@wruggeri, if you can have a look at the code of the class and you do not see any issue, feel free to remove the comment in your PR and maybe put at its place a link to this issue that I'll close.

Cheers,
D.