Quality of source code

Hi @,



    I'm sorry but I do not like the JME source code as well. I turn on the PMD plugin with the rulessets from Maven script. There some quality issues for developers. For example:


  1. ArrayList<Object> should be List<Object>.
  2. if (obj != data) should be if(obj.equals(data)).

    3…

    just turn on Checkstyle or PMD plugin



    Maybe is it my professional deformation but I think quality of source code it is very important for project.



    for example see: Effective Java (2nd Edition) (The Java Series) (Paperback) by Joshua Bloch (Author)





    Andriu



    Ps: please do not kill me for this :wink:


You are totally right. It's not your professional deformation - it's just rules for normal (basic? :slight_smile: ) programming in Java

andriu said:


1. ArrayList<Object> should be List<Object>.

Unless you want acces to methods like ensureCapacity(),  toArray(), etc. All important if you're doing peformance sensitive applications



2. if (obj != data) should be if(obj.equals(data)).

"obj != data" takes a fixed amount of cycles, while obj.equals(data) can take as long as the implementation makes it. In some cases, pretty damned long! (for example comparing two ByteBuffers). Eg changing to this when doing RenderState compares would probably dip the FPS in some scenes by several dozen percent. Escp texture state comparing is one of the "hot spots" in the code right now. You could optimize all your equals methods to do reference checking (blegh), to lessen the effect of that (still gonna get a lot of method overhead though).. but then either you're breaking the contract of equals (if you only do a reference check).

We're building a perfomance sensetive game engine here, so you better turn off any PMD rules that can have performance implications.

Also, as a.. professional, don't just follow The Rules Of the Machine, use your head instead of blindly following what some plugin tells you..

You could be right that there are some bad sections in the code, but your two examples are more like choices on how to do certain things than bad design (as posted by llama).

There are definitely sections where we can follow more "best practices" rules.  The thread pointing out EnumSet is one good example.  That said, we are not writing your typical Java application, so we will tend to break some rules in preference for speed.  Of course, you are welcome to propose specific code changes you feel would be beneficial.

Hi



yes, you have right. The optimization is very important in the JME. I am not looking for errors or bad source code. I am just learning the JME source code and I have many questions.


  1. About the ArrayList I am not sure :)  I think using the interfaces is the good way ( not always ).

    for example interface List have method toArray(),…


  2. if (obj != data) should be  if (obj != data)  :wink:



    I was looking for ensureCapacity method. I found the class (com.jme.util.geom.nvtristrip.FaceInfoVec) that use this method. And maybe this note is useful:



    class: com.jme.util.geom.nvtristrip.Stripifier

    method: buildStripifyInfo



    line 127 faceInfos.reserve(numIndices / 3);

    line 134 int numTriangles = numIndices / 3;

    maybe could be one line for this calculation. For example:



    // iterate through the triangles of the triangle list

    int numTriangles = numIndices / 3;



    int numIndices = indices.size();

    faceInfos.reserve(numTriangles);





    bye,

    Andriu

The ArrayList is a wrapped array, its the fastest List implementation, if the code used any other type of List, it would be bad for performance. So using ArrayList and not List actually makes sure, that you dont make the mistake of using some less optimal List implementation, and later wonder why your game runs badly. Mixing List implementations is bad, some operations involve copying elements one-by-one, while for example copying ArrayList element ranges is implemented with a single System.arrayCopy. Remember, the code is subordinate to the needed functionality (and performance), not the other way around.


andriu said:

2. if (obj != data) should be if(obj.equals(data)).


If you know what you're doing, this is not wrong. It is a reference compare, meaning, do theese two references point to the same object. A call to equals compares the contents of the referenced objects, if they don't point to the same objects. So if you only want to know, if those two references point to the same object, a direct compare is the preffered way to go in terms of performance.

I think we should move to the base interface classes where it doesnt have any implications (and it doesnt usually). It's always good to make it easy to change implementations of things like that. For example i often make my own stripped ArrayList classes with faster capacity checks etc.

So here are my 2 cent:



I found FindBugs much more useful than PMD, because it is much more precise about things that might be real errors, while PMD cares more about "style". Out of curiosity I set FindBugs on JME2 and found 1015 "bugs". From my experience and considering the heavy optimization, I would expect that 150~200 of them are real problems. I'd suggest to look first at the FindBugs report before going into the PMD report at all. If someone wants the data, I can send it (but it took just 10~15 minutes to generate it).

I'll have a look with their eclipse plugin.  My current Eclipse levels show 1105 warnings, but almost all of them are "trivial" like no serialVersionID (800+ of the warnings) or use of raw types (not providing Generics information on ArrayLists, etc.) (200+).  The rest are unused field/variable/methods.

For eclipse, i use Checklipse (MVMSoft). Its a plugin for CheckStyle which contains something similar like FindBugs as well. It's always good to adhere to some rules, it makes better code and less bugs.

vear said:

The ArrayList is a wrapped array, its the fastest List implementation...


This is not actually true.  In some cases ArrayList is dang fast, but consider the following jUnit test:

   @Test
   public void test() {
      List<String> list = new ArrayList<String>();
      long time = System.currentTimeMillis();
      for (int i = 0; i < 100000; i++) {
         list.add("value " + i);
      }
      while (list.size() > 0) {
         list.remove(0);
      }
      System.out.println("Time: " + (System.currentTimeMillis() - time) + "ms");
   }



Take a wild guess at how long that takes to complete?  For me it takes 11 seconds.  Now, simply change that to LinkedList and you end up with 228ms.  Granted this is a slanted test case that displays one of the major underlying flaws with ArrayList, but proves the point I think quite well. ;)  This is one of the reasons I actually avoid ArrayList if I'm doing any heavy removing / re-adding.  In fact, I avoid lists altogether in favor of Queue when possible these days.

you cannont say in general which is better or worse. all of this hevily depends on how you use the specific list.



would you have a list of exactly 10,000 Quads that you are accessing randomly and often with .get(r.nextInt(10000)), the linked list will be far worse performing than the ArrayList.



I recommend you dump that topic, it won't lead anywhere  :wink:

dhdd said:

you cannont say in general which is better or worse. all of this hevily depends on how you use the specific list.

would you have a list of exactly 10,000 Quads that you are accessing randomly and often with .get(r.nextInt(10000)), the linked list will be far worse performing than the ArrayList.

I recommend you dump that topic, it won't lead anywhere  ;)


Notice that I never said LinkedList was the better choice in all circumstances, but rather that "...in some cases ArrayList is dang fast..."  :-o

point well taken. I guess what i tried to say is that it really depends on how you want to use it (and that this discussion can never be decided in favor for one or the other)  ;).

darkfrog said:


Notice that I never said LinkedList was the better choice in all circumstances, but rather that "...in some cases ArrayList is dang fast..."  :-o


I would change that example code, put elements in the opposite order into the list, remove them from the end, and still use the ArrayList (or a stripped version with faster checks in it, as MrCoder said).  :D

But, yeah we are talking pure theory here, and using ArrayList everywhere wasnt my main point either.

Since the example you wrote depends on the behavior of the LinkedList, why would you let it change to ArrayList? My point was, i find it better to declare my lists as the actual implementation type, if i rely on that implementations behavior. If i'm depending on ArrayList behavior, i dont want my optimized code to be broken by giving it a LinkedList to work on. Got the point?
renanse said:

My current Eclipse levels show 1105 warnings, but almost all of them are "trivial" like no serialVersionID (800+ of the warnings) or use of raw types (not providing Generics information on ArrayLists, etc.) (200+).


These bugs are all trivial, but some of them are nevertheless serious:

JOGLTExtureRenderer 913...


   case Intensity32F:
       source = GL.GL_INTENSITY;  //<


no break, falls through
   case Luminance32F:
       source = GL.GL_LUMINANCE;
       break;



X3dToJME 460...


    private Node getChildNode(Node node, String name) {
        Node child = node.getFirstChild();
        while (child != null && child.getNodeName() != name) {   //<


String test may fail
            child = child.getNextSibling();
        }
        return child;
    }



SchemaDouble 134... (similar in SchemaFloat)


  public boolean booleanValue() {
    return !(value==0 || value==Double.NaN);  //<---- the correct test for NaN is value != value
  }



I see overridden parameters, null dereferences, 20 potential multithreading bugs etc., and I'd suggest to take these issues serious. And I'm sure that you'll also catch several errors after converting the raw types to generics (especially when the code is still in flow like in JME2).

BTW: After reading your response I'm not really sure wether the eclipse plugin shows the same things as the standalone FindBugs.

No, those were just eclipse warnings.  I installed the plugin and you may notice that fixes have been going in.

I think this will help a lot to increase the reliability of JME. Thank you  :smiley: