[Engine-devel] StorageHelperDirector "factory" and StorageType

[Eli Mesika]

----- Original Message -----
> From: "Oved Ourfalli" <ovedo at redhat.com>
> To: "Yair Zaslavsky" <yzaslavs at redhat.com>
> Cc: "engine-devel" <engine-devel at ovirt.org>
> Sent: Sunday, April 22, 2012 1:40:14 PM
> Subject: Re: [Engine-devel] StorageHelperDirector "factory" and StorageType
> 
> 
> 
> ----- Original Message -----
> > From: "Yair Zaslavsky" <yzaslavs at redhat.com>
> > To: "engine-devel" <engine-devel at ovirt.org>
> > Sent: Thursday, April 19, 2012 7:34:34 PM
> > Subject: [Engine-devel] StorageHelperDirector "factory" and
> > StorageType
> > 
> > Hi all,
> > StorageHelperDirector.java implements a mechanism that creates
> > instances
> > of classes implementing IStorageHelper based on String value of
> > StorageType enum.
> > Although the InitializeHelpers method has an "automated" mechanism
> > for
> > creating the helpers, I think it causes some bad practice -
> > It creates a tight coupling between the StorageType values and the
> > class
> > names of storage helpers, and a result of that, Java naming
> > convention
> > is broken (we use upper case enum literals at StorageType which
> > yield
> > class names such as LOCALFSStorageHelper.
> > 
> > I think that this mechanism is an overkill, and during addition of
> > new
> > storage type, its easily to detect during development if one has
> > forgotten to add the helper to the factory (if done manually).
> > 
> > Thoughts about this issue are more than welcome
> > 
> 
> I agree that it is an overkill, especially because we don't add new
> storage types very often, and even if we do the programmer who added
> it will probably find out about it one way or another (while testing
> he will see an exception, leading him to the correct place).
> 
> If it doesn't sound reasonable to others then there are other
> solutions (only if we really really have to....):
> 1. Have the Enum contain also a "class name" or "class prefix name"
> for that specific type, which will be used instead of the enum
> value. It can be useful in cases in which there are a few classes
> needed for this type (might not be relevant to the specific case you
> described, but might be useful in other cases).
+1 (if we must...)
> 2. Make a checklist in the Enum of places to touch when changing the
> enum - hopefully the programmer will read it, and do as he is
> told... Also, this checklist should be updated as well when new
> interfaces are added, and I don't see how we can automatically
> update this checklist :-)
> 3. Assuming there is a tester to the factory, we can add an assert
> that checks how many values are there in the enum. Once the enum is
> extended, the test will fail, and we will know that we need to check
> for related things (we can put a comment in the tester, specifying a
> "checklist"). - from my experience, in most cases it will end in the
> programmer doing everything needed, then failing on this test and
> fixing it (updating the number of values....)
> 
> Compile-time assertions are useful in such cases, but didn't find
> such an option in Java (perhaps we should move the engine to C++ ;-)
> ).
> 
> I'd go with option #1 (again... only if we really need to provide an
> "automatic factory").
> 
> Oved
> 
> > Yair
> > _______________________________________________
> > Engine-devel mailing list
> > Engine-devel at ovirt.org
> > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > 
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>