On 2/2/07, Stephane Bailliez <[EMAIL PROTECTED]> wrote:
Xavier Hanin wrote: > > My idea was to distinguish constructors (I defined two actually: one > with no > arguments, and one with all options as parameter) from factory method > which > actually process the argument to find what values should be for > options (the > settings is not part of the attributes of DeliverOptions, it's only > used to > give default values to the actual attributes). Another thing is that > factory > methods have the advantage to be more flexible (as explained in the > excellent "Effective Java Programming" you've certainly read). But in > this > case I don't see how it could really help, so if several people prefer > constructor, I'll go with it, no problem. > My main concern for this type of thing is: 1) you introduce dependencies into another component (IvySettings for instance)
I'm sorry I must be tired but I don't see how using a factory method instead of a constructor introduces a dependency. Are we really talking about the same thing? 2) introducing this method, basically means that your class is final and
inheriting it is not an option at all.
I admit that in case of DeliverOptions I don't see any reason why we'd want to subclass it, since it doesn't define any behavior, it's only used to group attributes. So I'm fine with using a constructor instead of a factory method, but I still don't see the advantage. Overall in this specific case dependencies and constraints about the
model are introduced while you get no benefit in return. Indeed a DeliverOptions is actually created when you deliver...which means you do it only once in Ivy class.
Once again I'm not sure we're really talking about the same thing, but the benefit I see when using a class to store a set of parameters is to have a cleaner and more readable API (methods with less arguments) and more manageable (it's easy to add a new parameter without breaking the API). But I'm pretty sure you agree that methods should have too many arguments, as you suggest it's a bad thing for constructors (below). But maybe you have another solution than introducing a class like DeliverOptions?
I'm not sure to see what you mean. If I provide an IvySettings object, > I can > have good default values for my attributes. Otherwise I can't, for the > cache > for example. So does it mean I should avoid the empty constructor to > be sure > a DeliverOptions won't ever be badly set? As long as the contract is clearly established, to me that's pretty much fine... After all that's what the bean and dependency injection is about
+1 And I'm no real fan to full constructor injection because this is
unreadable and people tend to stack all possible properties which becomes a nightmare when most of them are the same type, it's unreadable and error prone. In general arguments in the constructors are the 'critically needed' one and maybe another one or 2 constructors with 1 or 2 additional arguments as a helper.
I agree, and even with the constructor I introduced with all parameters the problem is that if we add one additional parameter we will have either to: - add it to this constructor, thus modifying the API or DeliverOptions, - duplicate to have both the previous and the new one, thus running in the same problem we have today with the huge number of methods (see resolve for example) doing the same thing with a slight difference in arguments - let it as is so that the new parameter can only be set by a setter, thus breaking the consistency of the API. So no solution is good, hence I agree with your arguments, and I should review the constructors of DeliverOptions as you suggest. The problem you're having with the cache is because the cache is not
fully abstracted (or at the very least the 'repository) all while it should be totally transparent to most objects...well that would be a pain to fix all that but there's no reason to actually have the logic deported everywhere and force you to pass the cache reference everywhere.
Yes, this is a real problem. Another problem is that from the beginning it's possible to specify the directory to use as root for the cache in almost all method calls, instead of always using the default cache configured in the settings. So this directory everywhere, instead of using only the settings which are usually available everywhere. A real pain, for something that I doubt is even used :-( Anyway, cache management should really be reviewed and improved, but I don't think it's directly related to the topic of method arguments refactoring. - Xavier -- stephane
