[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 Yair
----- Original Message -----
From: "Yair Zaslavsky" <yzaslavs@redhat.com> To: "engine-devel" <engine-devel@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). 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@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
----- Original Message -----
From: "Oved Ourfalli" <ovedo@redhat.com> To: "Yair Zaslavsky" <yzaslavs@redhat.com> Cc: "engine-devel" <engine-devel@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@redhat.com> To: "engine-devel" <engine-devel@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@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
participants (3)
-
Eli Mesika -
Oved Ourfalli -
Yair Zaslavsky