9 Replies Latest reply on May 18, 2011 8:23 AM by clebert.suconic

    @Override everywhere

    borges

      Hi folks,

       

      HornetQ trunk is now compiled using Java6. Since Java6, it is possible to add @Override also to methods overriding interfaces (that includes interface methods overriding other interface methods). I would like to make the case for us to start consistently using @Override also for interfaces.

       

      At Effective Java, Josh Bloch makes the following points about Override:

       

      Therefore, you should use the Override annotation on every method declaration that you believe to override a superclass declaration. There is one minor exception to this rule. If you are writing a class that is not labeled abstract, and you believe that it overrides an abstract method, you needn’t bother putting the Override annotation on that method. In a class that is not declared abstract, the compiler will emit an error message if you fail to override an abstract superclass method. However, you might wish to draw attention to all of the methods in your class that override superclass methods, in which case you should feel free to annotate these methods too.

       

      [ ... snip lots of good text ...]

       

      [...] it is worth annotating all methods that you believe to override superclass or superinterface methods, whether concrete or abstract.

       

      (Joshua Bloch, Effective Java 2nd Edition, Item 36 "Consistently use the Override annotation") ( <-- read this book!)

       

      I would like to make the case that we should also use override even for concrete classes overriding interface methods (the minor exception to the rule Bloch talks about).

       

      [...]

       

       

      There are correctness cases for using override:

      1. to avoid not overriding a method
      2. to avoid unintencional method override


      1. Avoid not overriding a method, is easy to illustrate (and HornetQ code already practices it):

       

      class B extends A {

      public void doSomething() { // etc }

      }

       

      class A {

      public void doSomeThing() { //etc }

      }

       

       

      2. avoid unintentional method override, is the use case improved with Java6.

       

      This does not appear in new code implementing interfaces, but it becomes a problem when code quantity and complexity increase, and people start refactoring it

       

      We have class A implementing (or not) some interfaces, and with ~ 2000 loc.

       

      class A implements Foo, Bar {

       

        [1000 loc]

       

        public void doFooThis() { // overrides a method from Foo }

        public void doThis() { // an innocent public non-overriding method }


        [1000 loc]


      }

       

      This class now needs to implement interface Baz with a bunch of methods, one of which is void doThis() (!!)

       

      interface Baz {

      [ ... bunch of methods ... ]

       

      void doThis();

      }

       

      class A implements Foo, Bar, Baz {

       

      [1000 loc]

       
       
      public void doFooThis() { // overrides a method from Foo }
        public void doThis() { // an  accidentally overriding method. NO WARNING! }


        [1000 loc]

       

         // A bunch of new methods are added...

      }

       

      Now we had a name collision, the developer was in a hurry, and neither the IDE nor the compiler could save us.

       

      A related operation may occur when trying to split a class, say we need to split class A into (1) A implements Bar, Baz  and (2)  AFoo implements Foo.

       

      class A implements Bar, Baz {

       

      [1000 loc]

       
       
      public void doFooThis() { // left over method from Foo. NO WARNING! }
        public void doThis() { // a public accidentally overriding method }


        [1000 loc]

       

         // A bunch of new methods are added...

      }

      class AFoo implements Foo {

        // code...

      }

       

      In both cases, if we had all methods annotated with override, we could have spotted the issues.

       

      I know people are supposed to pay attention to code changes, but just like we need to add unit tests that do sanity tests, IMHO we should have @Override annotations in places that make sense.

       

      [...]

       

      In the short foreseeable future, I intent to add a checkstyle configuration to the project, and add Hudson reports for it. If you folks agree, I will turn on the checks for @Override for interfaces there (and in the default Eclipse configuration as well).

        • 1. @Override everywhere
          clebert.suconic

          When you implement an interface, you are implementing, you are not overriding:

           

          ** every method declaration that you believe to override a superclass declaration **

           

           


          I'm not really sure it would be a good thing to use @override on interfaces.

           

           

           

           

          We always have one Impl class for every interface anyways.

           

           

          However, when extending classes, it should be mandatory to use @override from abstract classes, and I'm sure there are a few missings.

          • 2. @Override everywhere
            dmlloyd

            I see no point in using @Override.  It adds a huge amount of noise for a very marginal benefit.  That's my take on it...

            • 3. @Override everywhere
              ataylor

              I agree with david, i just can't see any benefit. Theres a dozen other ways i will write dodgy code before any of the above mentioned. tbh if you end up making any of these slip ups then theres typically something wrong with your abstraction anyway.

              • 4. @Override everywhere
                clebert.suconic

                +1

                • 5. @Override everywhere
                  jason.greene

                  I find @Override extremely annoying for inner classes. Otherwise I am indifferent about it, and usually leave it because my IDE generates it.

                   

                  One side comment, effective Java is a list of opinions that are often misinterpreted as fact. Some things he says I whole heartedly agree with (e.g. preferring immutable types, avoiding inheritance and so on). Other things I completely disagree with (e.g. the design of java thread locals). Like anything, It's always good to have a healthy dose of skepticism when you read this stuff.

                  • 6. @Override everywhere
                    borges

                    > Theres a dozen other ways i will write dodgy code before any of the above mentioned. tbh if you end up making any of these slip ups then theres typically something wrong with your abstraction anyway.

                     

                    There are a bazillion of ways of writing dodgy code, and that is why we need to program defensively. That is the very reason we also add simple sanity unit-tests.

                     

                    Do notice that right now there are a bunch of obvious[1] easy to address and small quality problems [2] in code base, but just like when there is "something wrong with your abstraction", to be able to address those you will first need to (somehow) become aware of their existence.

                     

                    [1] "Obvious" in the sense that there are fully automated ways to detect and deal with them, but for one reason or another the tooling is not in place.

                     

                    [2] Such as:

                    1. code with faulty .equals(), hashCode(), or .compare(,). (I fixed 2 of those, there are more.)

                    2. loads of unused imports left over everywhere.

                    • 7. @Override everywhere
                      borges

                      > effective Java is a list of opinions that are often misinterpreted as fact.

                       

                      IMO you do Effective Java an injustice by calling it a list of opinions. It is a lot more than that, it is a well justified, and argued for list of guidelines (or 'opinions' if you like).

                       

                      Not that I always agree with them, most of my post was to argue that there are good reasons to address the "small exception" that Bloch saw no reason to address.

                      • 8. @Override everywhere
                        clebert.suconic

                        Well, even josh bl ock didnt say anything about override on implements. I wouldn't do it, and I wouldn't force it. Overrides is for overrides.

                         

                        Regarding the "quality" of the code we do have hashcode and equals on classes that are meant to be collected. The cases we only have equals is for assertions on testcases. This will only serve the purpose of cleaning reports.

                         

                        The issue of changing formatting, or small warnings is the risk of losing diffs between the branches. It's already difficult with the source trees being different.

                         

                        We used to have the project settings on eclipse enforcing formatting rules, final variables And a few other rules.  We will need that back in place (you have added it, haven't you?)

                         

                        Anyway... Let's get maven done first. Then we can cleanup find bugs later.

                        • 9. @Override everywhere
                          clebert.suconic

                          On my previous post the iPad changed collection by collected.  We use equals and hash code on classes that a being used on hash maps and hash sets.