Droids
  1. Droids
  2. DROIDS-106

Introduce Guava (the former google collections) into the droids project

    Details

    • Type: Wish Wish
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.1.0
    • Fix Version/s: 0.1.0
    • Component/s: core
    • Labels:
      None

      Description

      Google Guava is becoming a standard extension to the collection framework and provides some very useful features (besides the obvious collection work which is extensive), such as Preconditions (which would standardize the way argument checking is done).
      As far as I know, many other Apache projects use Guava (Mahout is one).

      1. DROIDS-106.patch
        7 kB
        Eugen Paraschiv

        Activity

        Hide
        Richard Frovarp added a comment -

        You can find the library over here: http://code.google.com/p/guava-libraries/

        I used Google Collections in my Droids implementation (was done 6 months ago and haven't had a chance to update). I'm using Guava in another project that needs to do set operations and makes good use of some of the other collection types. One of the biggest advantages that Guava has over commons-collections is that it supports generics.

        Show
        Richard Frovarp added a comment - You can find the library over here: http://code.google.com/p/guava-libraries/ I used Google Collections in my Droids implementation (was done 6 months ago and haven't had a chance to update). I'm using Guava in another project that needs to do set operations and makes good use of some of the other collection types. One of the biggest advantages that Guava has over commons-collections is that it supports generics.
        Hide
        Richard Frovarp added a comment -

        While I support the use of Guava, I do think that it should be introduced in a patch that makes use of the library. Otherwise we have a dependency that is never used.

        Show
        Richard Frovarp added a comment - While I support the use of Guava, I do think that it should be introduced in a patch that makes use of the library. Otherwise we have a dependency that is never used.
        Hide
        Eugen Paraschiv added a comment - - edited

        I agree, this issue will be necessary for a new issue about cleaning up error checking and preconditions by using the Preconditions class provided by Guava.

        Show
        Eugen Paraschiv added a comment - - edited I agree, this issue will be necessary for a new issue about cleaning up error checking and preconditions by using the Preconditions class provided by Guava.
        Hide
        Otis Gospodnetic added a comment -

        Let's wait for Eugen's patch for precondition checking and see.

        +1 for this if it makes code cleaner.

        Show
        Otis Gospodnetic added a comment - Let's wait for Eugen's patch for precondition checking and see. +1 for this if it makes code cleaner.
        Hide
        Eugen Paraschiv added a comment -

        This patch includes the work of changing the existing precondition checking in droids-core from manual to Guava based checking. Things to note here are: I kept the exact type of exception that was being thrown in the manual checks, but later one we may want to check for null with the dedicated static methods in the Preconditions class (checkNotNull for instance). Also, there are some other parts of the code that would benefit from additional checks, but this patch only focuses on replacing the existing checks not writing more. Hope this helps to display a simple use case for the readability and general improvements that Guava brings to the code base.

        Show
        Eugen Paraschiv added a comment - This patch includes the work of changing the existing precondition checking in droids-core from manual to Guava based checking. Things to note here are: I kept the exact type of exception that was being thrown in the manual checks, but later one we may want to check for null with the dedicated static methods in the Preconditions class (checkNotNull for instance). Also, there are some other parts of the code that would benefit from additional checks, but this patch only focuses on replacing the existing checks not writing more. Hope this helps to display a simple use case for the readability and general improvements that Guava brings to the code base.
        Hide
        Otis Gospodnetic added a comment - - edited

        Thanks Eugen. I looked at the patch. What exactly does this buy Droids? (and I don't mean this in a bad tone at all!)

        Here is an example change:

        -    if (ssock == null) {
        -      throw new IllegalStateException("not running");
        -    }
        +    Preconditions.checkState(ssock != null, "not running" );
        

        This seems about the same in terms of the amount of code or clarify/readability.

        Of course, maybe there are some other ugly bits in Droids that Guava could improve? I don't know.

        Show
        Otis Gospodnetic added a comment - - edited Thanks Eugen. I looked at the patch. What exactly does this buy Droids? (and I don't mean this in a bad tone at all!) Here is an example change: - if (ssock == null ) { - throw new IllegalStateException( "not running" ); - } + Preconditions.checkState(ssock != null , "not running" ); This seems about the same in terms of the amount of code or clarify/readability. Of course, maybe there are some other ugly bits in Droids that Guava could improve? I don't know.
        Hide
        Eugen Paraschiv added a comment -

        Well, having a consistent way of handling precondition checking and exceptions in general provides a few advantages:

        • one improvement it brings is code readability - Preconditions.checkState communicates intent more clearly than manual checking (you can now differentiate cleanly between checking state, checking argument, checking something is non-null, etc)
        • then, it ensures consistency - you make sure that the underlying code will always throw the same kind of exception, as opposed to deciding what to throw on a case by case basis.
        • then, manual checking means that a method that handles some logic has to change it's level of abstraction and throw exceptions by hand, thus going against a good practice of keeping a method at the same level of abstraction; Preconditions.check___ allows the code to be at a higher level of abstraction, allowing the method to focus on the logic rather than throwing some IllegalStateException by hand.
          And finally, while I submitted this patch with precondition checking as a simple first example of the Guava functionality, this is just one of the features it has. Guava is the former Google Collections, designed by the Joshua Bloch who created the standard Java Collection API, which makes Guava very collection focused and a natural extension to the standard collection API, and seeing how Droids makes and will make use of collections in many areas, it looks like a natural fit here as well.
          Hope this has helped.
          Thanks for the feedback, it's good to see activity on Droids.
          Eugen.
        Show
        Eugen Paraschiv added a comment - Well, having a consistent way of handling precondition checking and exceptions in general provides a few advantages: one improvement it brings is code readability - Preconditions.checkState communicates intent more clearly than manual checking (you can now differentiate cleanly between checking state, checking argument, checking something is non-null, etc) then, it ensures consistency - you make sure that the underlying code will always throw the same kind of exception, as opposed to deciding what to throw on a case by case basis. then, manual checking means that a method that handles some logic has to change it's level of abstraction and throw exceptions by hand, thus going against a good practice of keeping a method at the same level of abstraction; Preconditions.check___ allows the code to be at a higher level of abstraction, allowing the method to focus on the logic rather than throwing some IllegalStateException by hand. And finally, while I submitted this patch with precondition checking as a simple first example of the Guava functionality, this is just one of the features it has. Guava is the former Google Collections, designed by the Joshua Bloch who created the standard Java Collection API, which makes Guava very collection focused and a natural extension to the standard collection API, and seeing how Droids makes and will make use of collections in many areas, it looks like a natural fit here as well. Hope this has helped. Thanks for the feedback, it's good to see activity on Droids. Eugen.
        Hide
        Otis Gospodnetic added a comment -

        Thanks for taking the time to explain. I'll buy that.
        Patch applies cleanly, but mvn test from root doesn't pass after the patch. How about for you?

        Show
        Otis Gospodnetic added a comment - Thanks for taking the time to explain. I'll buy that. Patch applies cleanly, but mvn test from root doesn't pass after the patch. How about for you?
        Hide
        Eugen Paraschiv added a comment -

        What exactly is the error. I'm running it from root and everything passes.

        Show
        Eugen Paraschiv added a comment - What exactly is the error. I'm running it from root and everything passes.
        Hide
        Otis Gospodnetic added a comment -

        Looks like it's just CNFE:
        Caused by: java.lang.ClassNotFoundException: com.google.common.base.Preconditions

        Shouldn't mvn test fetch Guava?
        Do we need to add an entry for the maven repo where Guava lives?

        Show
        Otis Gospodnetic added a comment - Looks like it's just CNFE: Caused by: java.lang.ClassNotFoundException: com.google.common.base.Preconditions Shouldn't mvn test fetch Guava? Do we need to add an entry for the maven repo where Guava lives?
        Hide
        Eugen Paraschiv added a comment -

        Shouldn't be a problem, no, Guava is deployed in the main maven repo. What Maven are you using? And are you running offline?

        Show
        Eugen Paraschiv added a comment - Shouldn't be a problem, no, Guava is deployed in the main maven repo. What Maven are you using? And are you running offline?
        Hide
        Otis Gospodnetic added a comment -

        $ mvn -version
        Apache Maven 2.2.1 (rdebian-1)
        Java version: 1.6.0_21
        Java home: /opt/java/32/jdk1.6.0_21/jre
        Default locale: en_US, platform encoding: UTF-8
        OS name: "linux" version: "2.6.32-26-generic" arch: "i386" Family: "unix"

        Interestingly, I see Maven did install Guava in my Maven repo yesterday when I tried this. But mvn test from droids root still fails. BUT mvn test from droids-core PASSES.

        Show
        Otis Gospodnetic added a comment - $ mvn -version Apache Maven 2.2.1 (rdebian-1) Java version: 1.6.0_21 Java home: /opt/java/32/jdk1.6.0_21/jre Default locale: en_US, platform encoding: UTF-8 OS name: "linux" version: "2.6.32-26-generic" arch: "i386" Family: "unix" Interestingly, I see Maven did install Guava in my Maven repo yesterday when I tried this. But mvn test from droids root still fails. BUT mvn test from droids-core PASSES.
        Hide
        Eugen Paraschiv added a comment -

        I have tried the patch on 2 different systems (VMs) and the build works on all both of them. Perhaps it has something to do with your current setup (the issue seems to be related to the classpath, but it's hard to guess what it may be). Can someone confirm the patch is OK?
        Thanks.

        Show
        Eugen Paraschiv added a comment - I have tried the patch on 2 different systems (VMs) and the build works on all both of them. Perhaps it has something to do with your current setup (the issue seems to be related to the classpath, but it's hard to guess what it may be). Can someone confirm the patch is OK? Thanks.
        Hide
        Bertil Chapuis added a comment -

        I tried the patch and it works well for me. I havn't used Guava yet but it seems to be a nice set of tools.

        Show
        Bertil Chapuis added a comment - I tried the patch and it works well for me. I havn't used Guava yet but it seems to be a nice set of tools.
        Hide
        Eugen Paraschiv added a comment -

        Thanks for the quick answer. Next step - perhaps someone with commit rights can take a look and commit it.

        Show
        Eugen Paraschiv added a comment - Thanks for the quick answer. Next step - perhaps someone with commit rights can take a look and commit it.
        Hide
        Bertil Chapuis added a comment -

        +1

        I have the commit right, but since it's quit recent I'd love to see another feedback.

        Show
        Bertil Chapuis added a comment - +1 I have the commit right, but since it's quit recent I'd love to see another feedback.
        Hide
        Eugen Paraschiv added a comment - - edited

        Can someone please apply this patch? Perhaps we can get the final few 0.0.1 items committed and move to a release soon.

        Show
        Eugen Paraschiv added a comment - - edited Can someone please apply this patch? Perhaps we can get the final few 0.0.1 items committed and move to a release soon.
        Hide
        Richard Frovarp added a comment -

        The one thing I see here is a small bit of inconsistency. There is a checkNotNull method that is used once in the patch. Otherwise checkState and checkArgument are used to test for nullness. Reading the documentation, I think the checkArgument null checks should be replaced with checkNotNull. The patch as is works, we just might want to change some of those calls later to be more consistent.

        Show
        Richard Frovarp added a comment - The one thing I see here is a small bit of inconsistency. There is a checkNotNull method that is used once in the patch. Otherwise checkState and checkArgument are used to test for nullness. Reading the documentation, I think the checkArgument null checks should be replaced with checkNotNull. The patch as is works, we just might want to change some of those calls later to be more consistent.
        Hide
        Eugen Paraschiv added a comment -

        The reason behind the checkNotNull usage is this - my goal was to make sure that I wasn't breaking any APIs, so even if I wanted to use the checkNotNull method more often, I couldn't do that if I wanted to maintain the exact type of exception that was being thrown before the patch. Now in the case I did use checkNotNull, the exception being thrown was RuntimeException, so the fact that a NullPointerException is thrown there doesn't break the API.
        Now that being said, I would want to propose a new issue to use the check___ methods of the Guava Preconditions API to their full potential, later on.
        Hope that clarifies things a bit.
        Thanks a lot for the feedback.

        Show
        Eugen Paraschiv added a comment - The reason behind the checkNotNull usage is this - my goal was to make sure that I wasn't breaking any APIs, so even if I wanted to use the checkNotNull method more often, I couldn't do that if I wanted to maintain the exact type of exception that was being thrown before the patch. Now in the case I did use checkNotNull, the exception being thrown was RuntimeException, so the fact that a NullPointerException is thrown there doesn't break the API. Now that being said, I would want to propose a new issue to use the check___ methods of the Guava Preconditions API to their full potential, later on. Hope that clarifies things a bit. Thanks a lot for the feedback.
        Hide
        Bertil Chapuis added a comment -

        I committed this patch since it introduces really good practices for the future releases. An issue has been created for 0.0.2 to take Richard's comment into account. Thanks Eugen for your work.

        Show
        Bertil Chapuis added a comment - I committed this patch since it introduces really good practices for the future releases. An issue has been created for 0.0.2 to take Richard's comment into account. Thanks Eugen for your work.

          People

          • Assignee:
            Unassigned
            Reporter:
            Eugen Paraschiv
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development