Discussions

News: Hammurapi: Open source Java code inspection tool

  1. Hammurapi is an open source code inspection tool. Its release comes with more than 100 inspectors which inspect different aspects of code: Compliance with EJB specification, threading issues, coding standards, and much more.

    Hammurapi is primarily a tool for architects or senior developers who need to control quality of large codebases and quickly pinpoint high severity problems.

    It is unique in several ways:

    - Works on Java Language Metamodel, not on AST. It allows to easily create very powerful inspectors.

    - Provides waivers mechanism, which allows to mark a violation as 'approved' violation. E.g. not all empty catch blocks are bad.

    - Annotations framework allows to link custom code inspection reports to summary, package or individual file. See NCSS Annotation example.

    - Reports contain source code. It takes only 3 mouse clicks to get from summary page to the place in the code for a violation of a particular severity and type.

    - Highlights files according to violation with highest severity.

    Home page: http://www.hammurapi.org
    Downloads: http://www.hammurapi.org/content/download.html
    Self review: http://www.hammurapi.org/review/report.html
    Inspector list: http://www.hammurapi.org/content/inspectors.html
    ---
    Regards, Pavel.

    Threaded Messages (29)

  2. Can you explain to me why this And-expression class extends an Or-expression class? I don't get it.

    http://www.pavelvlasov.com/products/Jsel/doc/api/com/pavelvlasov/jsel/impl/expressions/LogicalAndImpl.html
  3. Hello,

    I already exlained it one of replies, just in case if you didn't read it. There is a production rule in Java Language Specification, chapter 19:
    ConditionalOrExpression:
    ConditionalAndExpression
    ConditionalOrExpression || ConditionalAndExpression

    Which says just this: OrExpression is either AndExpression or OrExpression || AndExpression, which means that AndExpression can be used in place of OrExpression (is kind of OrExpression). In Java language if some class can be used in place of another means that the first class is a subclass of the second (is kind of).
    ---
    Regards, Pavel
  4. I have not studied this tool in detail, but just from looking at their class hierarchy I can already assess that this design is dead-wrong:
    LogicalOrImpl
        LogicalAndImpl
            InclusiveOrImpl
                ExclusiveOrImpl
                    AndExpressionImpl
                         EqualityExpressionImpl
    I don’t know whether to laugh or to cry when I see something like this. This is exactly the reason why any serious commercial company would never even look at Open Source Projects except for rare well-known cases.

    Regards,
    Dmitriy Setrakyan
    xTier™ - Service Oriented Middleware for Java and .NET
  5. I don’t know whether to laugh or to cry when I see something like this. This is exactly the reason why any serious commercial company would never even look at Open Source Projects except for rare well-known cases.
    It's ok to cry, this is pretty bad. However, any commercial company can assess this product and can draw the same conclusion that you have. They can also choose not to use it based on that criteria, which is pretty great if you ask me. You cannot say the same thing for a commercial, closed-source application that is poorly coded. Furthermore, you can alway point out to the open source developer that they may want to look at a new way to do things and perhaps even contribute code. Just a thought.
  6. I have not studied this tool in detail, but just from looking at their class hierarchy I can already assess that this design is dead-wrong:
    Whats that discussion all about? You dont have to deal with the code, you just have to use it! If it would be a framework you build your code on I would unterstand your critics, but thats an external tool. So the features and ease of use are the real issues. Why not discuss that?

    regards
    martin
  7. Whats that discussion all about? You dont have to deal with the code, you just have to use it! If it would be a framework you build your code on I would unterstand your critics, but thats an external tool. So the features and ease of use are the real issues. Why not discuss that? regardsmartin
    That’s rather a shortsighted view. The underlying design has a direct impact on the overall quality and correctness of the product. Moreover, this is the code inspection tool. Why would I trust a tool that is designed so poorly to inspect my code?

    Regards,
    Dmitriy Setrakyan
    xTier™ - Service Oriented Middleware for Java and .NET
  8. That’s rather a shortsighted view. The underlying design has a direct impact on the overall quality and correctness of the product. Moreover, this is the code inspection tool. Why would I trust a tool that is designed so poorly to inspect my code?Regards,Dmitriy Setrakyan xTier™ - Service Oriented Middleware for Java and .NET
    Yeah, why would anyone trust their enterprise to poorly designed and buggy software. Excuse me while I get the latest Windows Bug/Fix pack. :)
  9. <blockquoteYeah, why would anyone trust their enterprise to poorly designed and buggy software. Excuse me while I get the latest Windows Bug/Fix pack. :)I think you are comparing apples and oranges. Software is always buggy and it always contains issues which can be fixed. But what we see in this tool crosses any acceptable borderline – to fix it you would have to entirely redesign it from scratch and completely rewrite the whole thing. I am not talking about some bugs here – I am talking about completely unprofessionally designed product.

    The whole point of Open Source is that people can actually look inside the guts of the product and make their judgments. I have made my judgment, you can make yours...

    Regards,
    Dmitriy Setrakyan
    xTier™ - Service Oriented Middleware for Java and .NET
  10. any serious commercial company would never even look at Open Source Projects except for rare well-known cases.
    Can you evaluate the source code of commercial software? They can be worst in commercial software.

    open source has nothing to hide.
  11. Expressions hierarchy[ Go to top ]

    Expressions type hierarchy was automatically generated from java grammar and it reflects a simple concept known as precedence rules. LogicalAnd has higher precedence than LogicalOr and this is why it is a subclass of LogicalOr, which means you can use LogicalAnd anywhere where LogicalOr is expected but not vice versa. If you can suggest better solution to express in Java such concept as precedence rules I would love to hear it.
  12. Expressions hierarchy[ Go to top ]

    Expressions type hierarchy was automatically generated from java grammar and it reflects a simple concept known as precedence rules. LogicalAnd has higher precedence than LogicalOr and this is why it is a subclass of LogicalOr, which means you can use LogicalAnd anywhere where LogicalOr is expected but not vice versa. If you can suggest better solution to express in Java such concept as precedence rules I would love to hear it.
    Well, at least that's _a_ reason. I think the problem for me is that I am used to the usual intepretation of inheritance as specialization. For example, Manager could extend Employee because in all cases a Manager _is_ an employee. However, an OrExpression is _not_ an AndExpression (or viceversa) by any stretch of the imagination. I can appreciate that you are trying to use inheritance to implement precedence or whatever it's called, but in Java you only have single inheritance: are you sure you want to use it for this?
  13. Expressions hierarchy[ Go to top ]

    You can find here http://java.sun.com/docs/books/jls/first_edition/html/19.doc.html that LogicalAndExpression IS LogicalOrExpression according to the following production rule (they call them differently):
    ConditionalOrExpression:
    ConditionalAndExpression
    ConditionalOrExpression || ConditionalAndExpression

    If you don't agree with this then drop an e-mail to James Gosling, Bill Joy or
    Guy Steele who wrote the above document.

    In Java I have multiple inheritance of interfaces. And, as one of speakers on The last Server Side Symposium said, concepts (read design) shall be expressed in interfaces. Classes are just implementation details.

    Inheritance to express precedence works quite well in RedundantParenthesisRule which finds things like a+(b*c) or return (33); I have a colleague who loves parenthesis and puts them everywhere.
  14. Expressions hierarchy[ Go to top ]

    You can find here http://java.sun.com/docs/books/jls/first_edition/html/19.doc.html that LogicalAndExpression IS LogicalOrExpression according to the following production rule (they call them differently):ConditionalOrExpression: ConditionalAndExpression ConditionalOrExpression || ConditionalAndExpressionIf you don't agree with this then drop an e-mail to James Gosling, Bill Joy orGuy Steele who wrote the above document.In Java I have multiple inheritance of interfaces.
    Point taken. Thanks for clearing that up.

    However, I wonder if this is only necessary due to the fact that you are using a strict LALR(1) grammar. Maybe using a more advanced parser generator you would end up with a different design.

    Example:


    precedence left PLUS, MINUS;
    precedence left TIMES, DIVIDE, MOD;
    precedence left UMINUS;

    expr_list ::= expr_list expr_part |
                  expr_part;

    expr_part ::= expr SEMI;

    expr ::= expr PLUS expr
                | expr MINUS expr
                | expr TIMES expr
                | expr DIVIDE expr
                | expr MOD expr
    | MINUS expr %prec UMINUS
                | LPAREN expr RPAREN
    | NUMBER
    ;


    http://www.cs.princeton.edu/~appel/modern/java/CUP/manual.html#intro
  15. Expressions hierarchy[ Go to top ]

    You know, I don't consider this deep inheritance tree to be so formidable evil as other authors in this thread do. I could have a method Expression.hasHigherPrecedence(Expression), but I chose to use inheritance as a marker of precedence. In my opinion if you can express something without adding methods then it is better to do so. Sun thinks in the same way. java.io.Serializable interface - it is a marker. They could require to provide isSerializable() method in Object, but ... I can say I followed their footsteps.

    I had a grammar and had to wrap the grammar into an object model it the fastest way. As I already pointed all Expression interfaces and implementations was autmatically generated by Transformica from a simplified grammar. After that I adjusted implementations of some of them.

    It works, this is the main point.

    It is like Java code generated by JSP compiler - if you look to generated code, it is terrible. But it is not written by humans and not to be read by humans. The same thing here - clients of the API generally shouldn't extends Expression interfaces, just use them.
  16. Expressions hierarchy[ Go to top ]

    It works, this is the main point. It is like Java code generated by JSP compiler - if you look to generated code, it is terrible. But it is not written by humans and not to be read by humans. The same thing here - clients of the API generally shouldn't extends Expression interfaces, just use them.
    Fair enough. Thanks for taking the time to respond because I was thinking of using this code but when I saw the design I started wondering what was going on. I think now I am more comfortable with it.

    Incidentally - I know I should RTM first - I am interested in doing semantic diffs for the purpose of doing code reviews. Do you have any input or interest in that? I would like to use semantic diffs to do create change logs automatically.
  17. Semantic diff[ Go to top ]

    Hello,

    I have an idea of something like similarity scanning which is somewhat similar to semantic difference: you take a piece of code and compare it with another piece of code. But comparison shall be 'parameterized' which means that variables and parameters names shouldn't matter, looping constructs shall be brought to a common denominator because e.g. 'for' can be expressed through 'while', ...

    But it is far future. Currently my primary goal is to add time dimension to Hammurapi so it could show how codebase evolves over time.
  18. Semantic diff[ Go to top ]

    Hello,I have an idea of something like similarity scanning which is somewhat similar to semantic difference: you take a piece of code and compare it with another piece of code. But comparison shall be 'parameterized' which means that variables and parameters names shouldn't matter, looping constructs shall be brought to a common denominator because e.g. 'for' can be expressed through 'while', ...But it is far future. Currently my primary goal is to add time dimension to Hammurapi so it could show how codebase evolves over time.
    Very cool idea. Actually I wasn't being that ambitious. I meant 'semantic' only in the sense that it ignores pure formatting differences. Perhaps I should be thinking of it as a 'syntax diff'. But even doing a diff of two trees I think presents arbitrary choices which must be resolved somehow.

    What I need is a good generalization of greatest-common-subsequence (I think that's what it's called) that works for trees.
  19. Semantic diff[ Go to top ]

    What I need is a good generalization of greatest-common-subsequence (I think that's what it's called) that works for trees.
    I think you shouldn't work on trees but rather on Jsel metamodel. For example, in compilation unit (file) order of import and type declarations declarations is not important. In type definitions (interfaces, classes) order of modifiers and fields/operations is not important. In fields and methods order of modifiers is also not important. Moreover method declaration in interface assumes 'public abstract'. I think all this stuff can be much easier handled at metamodel level.

    I recommend you to work on Jsel metamodel level down to Code (Method, Constructor, Static and Instance initializers). Inside Code it might be easier to work at AST level.
  20. Semantic diff[ Go to top ]

    I think you shouldn't work on trees but rather on Jsel metamodel.For example, in compilation unit (file) order of import and type declarations declarations is not important. In type definitions (interfaces, classes) order of modifiers and fields/operations is not important. In fields and methods order of modifiers is also not important.Moreover method declaration in interface assumes 'public abstract'. I think all this stuff can be much easier handled at metamodel level.I recommend you to work on Jsel metamodel level down to Code (Method, Constructor, Static and Instance initializers). Inside Code it might be easier to work at AST level.
    That's definitely a good approach, however it's also useful to know things like "developer A" swapped methods X and Y. So I was thinking of using ATSs because I don't want to drop any information.

    In fact, I would also want to keep the actual token list, in case you need that also.

    I guess to me the token list, the ast, the metamodel tree are all just trees. In fact, it would be nice to build further trees which are even more high-level. For example, you could have a tree that identifies certain code patterns. For example you could have a rule that an EJB has to be in its own package, so you could build a higher-level tree representation which has EJB nodes (and maybe other instance of patterns.)

    I also think that the design should not work just for Java, because another thing I would like to be able to do is cross-check items between Java and non-java files, e.g. property names in property files, SQL identifiers in .sql files, etc.

    So I was approaching the problem in a somewhat different way.
  21. Semantic diff[ Go to top ]

    That's definitely a good approach, however it's also useful to know things like "developer A" swapped methods X and Y.
    This is not a semantic difference because it doesn't change class behaviour. If you don't want to drop any information then you return to text comparison with only difference that whitespaces shall be ignored. Araxis merge does it pretty well.

    By the way, metamodel is not a tree, it is a graph.

    AST approach seems to be quite good for SQL.
  22. Expressions hierarchy[ Go to top ]

    Hi Pavel,
    Well, I guess you used some home-grown parser for the grammar b/c the end result looks, how can I put it, bizarre...
    Why don’t you tell us why this parser of yours is better then standard set of tools DFA/ANTLR or LEX/YACC and how generated parser is better then these.

    Regards,
    Nikita Ivanov.
  23. Expressions hierarchy[ Go to top ]

    Nikita,

    If you take a closer look at Jsel code you'll find that it uses ANTLR to build AST. Then this AST is used to build Java language metamodel. The end result looks like I wanted it to look - I can parse soruce and navigate through it. There are some wierd places in Jsel because it is half generated so there are files I've never looked in after generation.
    ---
    Regards, Pavel.
  24. Hello,

    I suggest you not to laugh or cry but think what could lead me to this kind of very deep inheritance tree. I know that deep inheritance trees are bad. There is even an inspector for Hammurapi in my TODO list which will check just this - very deep inheritance trees.

    I'll give you a hint: Read Java grammar and take a look at RedundantParenthesisRule implementation.

    If you still think that it is pretty bad, then ... just don't use it.
    ---
    Regards, Pavel.
  25. I have not studied this tool in detail, but just from looking at their class hierarchy I can already assess that this design is dead-wrong:
    LogicalOrImpl
        LogicalAndImpl
            InclusiveOrImpl
                ExclusiveOrImpl
                    AndExpressionImpl
                         EqualityExpressionImpl
    The inheritance hierarchy gets even more impressive in other places:
    java.lang.Object
     AbstractSearchable
      com.pavelvlasov.jsel.impl.LanguageElementImpl
       com.pavelvlasov.jsel.impl.InitializerImpl
        com.pavelvlasov.jsel.impl.expressions.ExpressionImpl
         com.pavelvlasov.jsel.impl.expressions.AssignmentExpressionImpl
          com.pavelvlasov.jsel.impl.expressions.LogicalOrImpl
           com.pavelvlasov.jsel.impl.expressions.LogicalAndImpl
            com.pavelvlasov.jsel.impl.expressions.InclusiveOrImpl
             com.pavelvlasov.jsel.impl.expressions.ExclusiveOrImpl
              com.pavelvlasov.jsel.impl.expressions.AndExpressionImpl
               com.pavelvlasov.jsel.impl.expressions.EqualityExpressionImpl
                com.pavelvlasov.jsel.impl.expressions.RelationalExpressionImpl
                 com.pavelvlasov.jsel.impl.expressions.ShiftExpressionImpl
                  com.pavelvlasov.jsel.impl.expressions.AdditiveExpressionImpl
                   com.pavelvlasov.jsel.impl.expressions.MultiplicativeExpressionImpl
                    com.pavelvlasov.jsel.impl.expressions.UnaryExpressionImpl
                     com.pavelvlasov.jsel.impl.expressions.UnaryExpressionNotPlusMinusImpl
                      com.pavelvlasov.jsel.impl.expressions.PostfixExpressionImpl
                       com.pavelvlasov.jsel.impl.expressions.PrimaryExpressionImpl
                        com.pavelvlasov.jsel.impl.expressions.NewImpl
                         com.pavelvlasov.jsel.impl.expressions.NewObjectImpl
                          com.pavelvlasov.jsel.impl.expressions.NewAnonymousObjectImpl

    I don't know if I have ever seen something like that!

    Regards,
    Jens
  26. Metricks could useful, however it is up to developers to decide if breaking a rule is OK in a particular case, or developers might be disagree with a rule. So, it would be nice if it was possible to say somehow: do not show _this_ and _that_ warning again. Then next time we run metrics we will see only new things and old ones, which we are going to address someday.
  27. By default Hammurapi has all inspectors turned on. You can create your own inspector sets by using nested <inspector> or <inspectors> elements in <hammurapi> task. Documentation how to do that is here. Support for incremental reviews, when you see only violations in changed code, is coming soon.
  28. Waivers[ Go to top ]

    Konstantin,

    There is a mechanism in Hammurapi to say "This violation is OK, don't show it to me again". It is called "waivers". You can read how to work with waivers in User Manual, chapter 5.
    ---
    Regards, Pavel.
  29. Using JSEL for parsing java code[ Go to top ]

    I am looking at parsing the java code with the aim of automating the transformation of the code from 1 framework to another.

    I would like to use JSEL. However, I can't see to find any doc. regarding the same. I tried looking at the Hammurapi docs., but they too don't cover much about it.

    Can someone guide me as to where I can find the information.

    also, does anyone have any other ideas regarding this.

    Cheers.
  30. Parsing Java file with Jsel[ Go to top ]

    The reply is one year late, but nonetheless: * http://wiki.hammurapi.biz/index.php?title=How_to_parse_Java_file_with_Jsel * http://wiki.hammurapi.biz/index.php?title=Parsing_a_file_with_Mesopotamia