7 Replies Latest reply on May 30, 2006 11:09 AM by Weston M. Price

    Refactoring

    Weston M. Price Master

      Currently, on the 4.0.5 code base I have done the following:

      Moved all pool related source into a newly created package

      org.jboss.resource.pool

      This removes the inner classes from the JBossManagedConnectionPoolMBean implementation and is overall, a cleaner way to organize things (at least in IMO).

      I would like to do something similar with the jdbc supporting stuff (ie ValidConnectionChecker, ExceptionSorter etc, etc) and get that stuff out of the adapter structure.

      I was thinking something along the lines of

      org.jboss.resource.jdbc.support

      and refactoring from there.

      Thoughts?

        • 1. Re: Refactoring
          Weston M. Price Master

          New packages:

          org.jboss.resource.jdbc.support

          org.jboss.resource.jdbc.support.vendor

          The former includes the

          Cached*
          BaseClasses/Interfaces


          The latter

          Vendor specific stuff.

          • 2. Re: Refactoring
            Adrian Brock Master

            You cannot change any interface or user level class.
            You break somebody else's compilation.

            That is unless you leave the interface in place and then
            do:

            public class ValidConnectionChecker extends org.jboss.new.location.ValidConnectionChecker
            


            I'm against refactoring just for the sake of it.
            i.e. no good reason like just changing names or packages.

            There are some people who have access to the codebase that
            I would like to go their IDE and put a big sticker over the refactoring
            button saying "leave it alone and do some real work!". :-)
            or
            "leave it alone until you learn to be more careful about breaking the build!".

            5.0.x (major version number change)
            is the place where you can change things incompatiblity.
            And then only if there is a good reason for it.

            • 3. Re: Refactoring
              Weston M. Price Master

              It's not refactoring for the sake of refactoring. In terms of the JDBC support, this should have never been in the "adapter" package to begin with.

              As far as the 'real work' comment, not sure about that one :-) And as far as I remember I haven't broken the build...well, that one time due to the 1.4/1.5 stuff. Making this easier to read, understand etc, etc. may actually alleviate some of the 'People just don't grok JCA' mantra.

              In terms of the pool classes, The JBossManagedConnectionPool/MBean remains in the same place. The only thing that has been moved is the BasePool and all it's children along with the management threads (PoolFiller, IdleRemover, ConnectionValidator) which should never be used directly anyway.



              • 4. Re: Refactoring
                Weston M. Price Master

                And, the build works fine (with the changes), testsuite runs fine etc. etc.

                I do actually do these things before checking stuff in you know :-)



                • 5. Re: Refactoring
                  Adrian Brock Master

                  It wasn't aimed at you, more at the webservices project. :-)

                  My aversion to refactoring still stands.
                  If it doesn't serve a good purpose.

                  On the jdbc support classes, I don't see how these
                  aren't a part of the resource adapter anyway?

                  1) They are specific to jdbc
                  2) The rar wouldn't work without them
                  3) The rar shouldn't depend upon classes from the connection manager

                  There is a support jar
                  jboss-common-jdbc-wrapper.jar
                  that should really be in the rar as well.

                  The reason it is not, is because we have multiple versions
                  of the jdbc rar.

                  * This is legacy of the JCA1.0 packaging where you could have
                  only one outbound MCF per rar
                  * The HA rars only exist separately because they were initially
                  experimental

                  If you do move them somewhere else, the integration apis
                  should be in a ".spi" package so it is clear which interfaces
                  are supported for user integration. But that isn't something
                  we can do in mid 4.0.x branch.

                  I prefer the package name ".plugin" for implementations of the spi.

                  • 6. Re: Refactoring
                    Weston M. Price Master

                    Right, 'refactoring' was probably a bad word :-) And I am in complete agreement with the frustration of doing an update and suddenly things are either completely removed, completely different etc, etc. Nothing worse than wasting an hour trying to figure exactly what happened.

                    I guess the comment was more directed towards improvements down the road. How we want to do things differently, how much of the JCA/MC stuff we can share across implementations. That sort of thing.

                    • 7. Re: Refactoring
                      Weston M. Price Master

                      Our JDBC RAR's should really be collapsed into one RAR as you point out. This would clean things up tremendously I think. We could also remove the DummyResourceAdapter stuff at the same time.