5 Replies Latest reply on Aug 10, 2007 9:12 AM by Adrian Brock

    AbstractController add/removeControllerContext

    Ales Justin Master

       

      "adrian@jboss.org" wrote:

      I note you've still not fixed some of the public methods you introduced
      on the AbstractKernelController that I complained about before.

      They need fixing otherwise somebody can break the entire state machine with injudicious
      (or malign) invocations on those methods.

      This is basic OO principles. All public methods should keep the invariant state
      of the underlying Object, there should be no backdoors. :-)


      Actually it's not AbstractKernelController, it's AbstractController, which complicates things - different package then ScopedKernelController - otherwise I could use package protected. :-(

      Looking at the code, I don't see how else I can move context from underlying controller into scoped one.
      Except for changing the whole impl, putting the scope information into base controller concept - which doesn't look that trivial to do. :-)

      Any ideas how to keep the current impl and at the same time remove this ugly public backdoors?

      e.g. ScopedKernelController extends AbstractKernelController extends ScopedController extends AbstractController
      And make those methods package protected?
      Too ugly and outside concept?


        • 1. Re: AbstractController add/removeControllerContext
          Ales Justin Master

           

          "alesj" wrote:

          e.g. ScopedKernelController extends AbstractKernelController extends ScopedController extends AbstractController


          If I do this it works and nothing is public.

          ScopingKernelController has package protected methods:
           void addControllerContext(KernelControllerContext context)
           {
           super.addControllerContext(context);
           }
          
           void removeControllerContext(KernelControllerContext context)
           {
           super.removeControllerContext(context);
           }
          
           void release()
           {
           getParentController().removeController(this);
           setUnderlyingController(null);
           setParentController(null);
           parentKernel = null;
           }
          

          since it is only accessed in PreInstallAction, which is in the same package.

          The only change to AbstractKernelController is that it extends ScopedController instead of AbstractController.

          This is how ScopedController looks like:
          /**
           * Scoped controller.
           *
           * @author <a href="ales.justin@jboss.com">Ales Justin</a>
           */
          public abstract class ScopedController extends AbstractController
          {
           private AbstractController underlyingController;
          
           public void setUnderlyingController(AbstractController underlyingController)
           {
           this.underlyingController = underlyingController;
           }
          
           protected boolean isScoped()
           {
           return underlyingController != null;
           }
          
           protected void addControllerContext(ControllerContext context)
           {
           if (isScoped())
           {
           underlyingController.removeControllerContext(context);
           context.setController(this);
           registerControllerContext(context);
           }
           else
           super.addControllerContext(context);
           }
          
           protected void removeControllerContext(ControllerContext context)
           {
           if (isScoped())
           {
           unregisterControllerContext(context);
           context.setController(underlyingController);
           underlyingController.addControllerContext(context);
           }
           else
           super.removeControllerContext(context);
           }
          
          }
          


          It has protected methods, so that they can be accessed in ScopedKernelController.

          And AbstractController now has package protected methods:
           void addControllerContext(ControllerContext context)
           {
           registerControllerContext(context);
           }
          
           // TODO This api looks broken and unsafe see above
           void removeControllerContext(ControllerContext context)
           {
           unregisterControllerContext(context);
           }
          


          Is this OK?
          Or too ugly hack?

          • 2. Re: AbstractController add/removeControllerContext
            Ales Justin Master

            + added missing write locks.

            • 3. Re: AbstractController add/removeControllerContext
              Adrian Brock Master

               

              "alesj" wrote:

              Is this OK?
              Or too ugly hack?


              That's the way I told you to do it before.

              The whole point of protected and "friend" in c++ is to allow restricted access
              to implementation details, but you need to make sure the access makes sense.

              i.e. Is it possible that the implementation will change down the road making these
              methods unsupportable?

              • 4. Re: AbstractController add/removeControllerContext
                Ales Justin Master

                 

                "adrian@jboss.org" wrote:
                "alesj" wrote:

                Is this OK?
                Or too ugly hack?

                That's the way I told you to do it before.

                To do an ugly hack? :-)

                "adrian@jboss.org" wrote:

                The whole point of protected and "friend" in c++ is to allow restricted access
                to implementation details, but you need to make sure the access makes sense.

                i.e. Is it possible that the implementation will change down the road making these methods unsupportable?

                If/when the whole scoping impl changes, it will be really easy to strip out the current impl - simply removing the Scoped(Kernel)Controller and re-connect the AbstractKernelController with AbstractController.
                Since those two classes + PreInstallAction are the only one's who know what the scope is.

                • 5. Re: AbstractController add/removeControllerContext
                  Adrian Brock Master

                   

                  "alesj" wrote:

                  Since those two classes + PreInstallAction are the only one's who know what the scope is.


                  And what if a user uses them?

                  Of course protected and package private methods is not something
                  somebody should expect to remain very stable unless the behaviour
                  has been explicitly documentated.