Ivy refactoring - step 3

[Xavier Hanin <xa...@gmail.com>]

Hi All,

I know I don't let you much time to give me feedback, but it always
possible to go back if you feel I do things in the wrong direction,
and since I have spare time for the moment, I make good progress on
Ivy refactoring.

I've "finished" the split of the Ivy class, it's now almost only a
Facade to other objects, most of them being "engines" dedicated to
each feature of Ivy.

I've also just checked in a modification on the API of the deliver
method. I took deliver as an example, but my plan is to follow the
same recipe for other methods, if feedback is positive.

What I've done is introduce a DeliverOptions class which stores most
of the parameters the deliver method was taking. Well, when I say
most, I mean all the parameters which have a default value. So now you
can easily call deliver with default values only, or setup a
DeliverOptions manually to configure the algorithm. The advantage I
see is that if we want to add a new option, we just have to add the
attribute with getter/setter, and the API of deliver does not change.
Another thing is that it improves readability, especially when you
have several boolean parameters, it's very difficult to know what
you're actually doing in your call.

For instance, compare these two calls:
Ivy 1.4:
ivy.deliver(mrid, "1.5", cache, "ivy-[revision].xml", "release", new
Date(), pdrResolver, false, true)

Ivy 1.5:
ivy.deliver(mrid, "1.5", "ivy-[revision].xml",
   new DeliverOptions()
        .setStatus("release")
        .setPubdate(new Date())
        .setCache(cacheManager)
        .setPdrResolver(pdrResolver)
        .setValidate(false)
        .setResolveDynamicRevisions(true));

The second is longer, but I think it's much cleaner too. And note that
it's long because we set all options, but if we want to use default
values for everything except the status, for example, then we can do:
ivy.deliver(mrid, "1.5", "ivy-[revision].xml",
    DeliverOptions.newInstance(settings).setStatus("release"));


The problem is that it changes the API, thus requiring a lot of work
to migrate tools using Ivy 1.4 API. That's why I've added a Ivy14
class, which provide a 1.4 compatible API, so that if you don't want
to use the new API, you can simply replace your instance of Ivy by an
instance of Ivy14, and you're done!

So, what do you think? Shall I continue on this way, and do that for
all main methods of Ivy? Or go back to the 1.4 API?

- Xavier

Re: Ivy refactoring - step 3

[Xavier Hanin <xa...@gmail.com>]

On 2/2/07, Stephane Bailliez <sb...@gmail.com> wrote:
>
> Xavier Hanin wrote:
> >
> > 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?
>
> It's me not being clear enough.
> I meant the fact that IvySettings is introduced within the factory
> method leads to this.


Ok, now it's clear and I agree. The point is that it's very useful to be
able to create and instance of DeliverOptions from an IvySettings instance,
so I think such a factory/constructor should exist somewhere. But if we want
to avoid the dependency, where should we put this factory?

> 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?
>
> Ah, I see probably where we don't understand each other.
> No I'm not saying that DeliverOptions is a bad idea, just the
> DeliverOptions.newInstance(IvySettings) because of this IvySettings that
> suddenly pops up as a dependency while it could very well not be there.


Ok, got it now!

>
> > 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.
>
> Oh I think it's just a manifestation of it, because cache or something
> else  when you are deep in the chain and suddenly find yourself in a
> position of 'oh s*, I need this information, how can I get it',
> generally we, humans tend to solve that problem as 'I'll give it as an
> argument to...'. Damn humans. :)


+1 But it's sometime difficult to find another way.

Oh and don't misunderstand me I really think what you've done is great
> because that will make it much easier to understand (I'm actually quite
> amazed at the speed you've been doing it), but I'm just no fan in
> introducing lots of methods that can be distracting in an API.


I agree, that's why my intent is to reduce the number of methods in the Ivy
class, to have only one method by feature, removing all those awful
combinations of possible arguments. I understand that in a sense I've moved
some of the combinations to DeliverOptions, but I think that if give only
two possible way to construct a DeliverOptions instance it would be fine:
1) no argument constructor, configure everything using setters
2) use an IvySettings object to configure with sensible defaults

Does it make sense?

Xavier

Not sure I'm much more clear, but I too have very tired eyes and cloudy
> brain. Sorry about that.
>
> -- stephane
>

Re: Ivy refactoring - step 3

[Stephane Bailliez <sb...@gmail.com>]

Xavier Hanin wrote:
>
> 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?

It's me not being clear enough.
I meant the fact that IvySettings is introduced within the factory 
method leads to this.

> 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?

Ah, I see probably where we don't understand each other.
No I'm not saying that DeliverOptions is a bad idea, just the 
DeliverOptions.newInstance(IvySettings) because of this IvySettings that 
suddenly pops up as a dependency while it could very well not be there.

>
> 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.

Oh I think it's just a manifestation of it, because cache or something 
else  when you are deep in the chain and suddenly find yourself in a 
position of 'oh s*, I need this information, how can I get it', 
generally we, humans tend to solve that problem as 'I'll give it as an 
argument to...'. Damn humans. :)

Oh and don't misunderstand me I really think what you've done is great 
because that will make it much easier to understand (I'm actually quite 
amazed at the speed you've been doing it), but I'm just no fan in 
introducing lots of methods that can be distracting in an API.

Not sure I'm much more clear, but I too have very tired eyes and cloudy 
brain. Sorry about that.

-- stephane

Re: Ivy refactoring - step 3

[Xavier Hanin <xa...@gmail.com>]

On 2/2/07, Stephane Bailliez <sb...@gmail.com> 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
>
>
>

Re: Ivy refactoring - step 3

[Stephane Bailliez <sb...@gmail.com>]

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)
2) introducing this method, basically means that your class is final and 
inheriting it is not an option at all.


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.

> 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

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.

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.

-- stephane

Re: Ivy refactoring - step 3

[Xavier Hanin <xa...@gmail.com>]

On 1/31/07, Stephane Bailliez <sb...@gmail.com> wrote:
>
> Xavier Hanin wrote:
> >
> >
> > ivy.deliver(mrid, "1.5", "ivy-[revision].xml",
> >   new DeliverOptions()
> >        .setStatus("release")
> >        .setPubdate(new Date())
> setPublicationDate() ?
> >        .setCache(cacheManager)
> >        .setPdrResolver(pdrResolver)
> What's a pdr ?
> >        .setValidate(false)
> >        .setResolveDynamicRevisions(true));
> >
> > The second is longer, but I think it's much cleaner too. And note that
> > it's long because we set all options, but if we want to use default
> > values for everything except the status, for example, then we can do:
> > ivy.deliver(mrid, "1.5", "ivy-[revision].xml",
> >    DeliverOptions.newInstance(settings).setStatus("release"));
>
> Why do you want to use a factory method ?


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.

Constructor works fine too and I think you should also take more
> advantage of having your object in a mostly known and relevant state for
> most properties and work from there.


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?

Also I do hate to stack several statements in one line as it is a PITA
> to debug.


You're right, but the API doesn't force you to put all on the same line :-)
I'll try to use multi line statements more often.

>
> > The problem is that it changes the API, thus requiring a lot of work
> > to migrate tools using Ivy 1.4 API. That's why I've added a Ivy14
> > class, which provide a 1.4 compatible API, so that if you don't want
> > to use the new API, you can simply replace your instance of Ivy by an
> > instance of Ivy14, and you're done!
>
> >
> > So, what do you think? Shall I continue on this way, and do that for
> > all main methods of Ivy? Or go back to the 1.4 API?
>
> Simplification and partitioning is good. :)
>
> I don't have much time to review it until the week end though.


It's fine, I think I'll let the API like that for the moment to wait for
review, and tomorrow I'll continue the refactoring with the IvyNode split. A
good day of headache in perspective :-)

Xavier

-- stephane
>

Re: Ivy refactoring - step 3

[Stephane Bailliez <sb...@gmail.com>]

Xavier Hanin wrote:
>
>
> ivy.deliver(mrid, "1.5", "ivy-[revision].xml",
>   new DeliverOptions()
>        .setStatus("release")
>        .setPubdate(new Date())
setPublicationDate() ?
>        .setCache(cacheManager)
>        .setPdrResolver(pdrResolver)
What's a pdr ?
>        .setValidate(false)
>        .setResolveDynamicRevisions(true));
>
> The second is longer, but I think it's much cleaner too. And note that
> it's long because we set all options, but if we want to use default
> values for everything except the status, for example, then we can do:
> ivy.deliver(mrid, "1.5", "ivy-[revision].xml",
>    DeliverOptions.newInstance(settings).setStatus("release"));

Why do you want to use a factory method ?
Constructor works fine too and I think you should also take more 
advantage of having your object in a mostly known and relevant state for 
most properties and work from there.

Also I do hate to stack several statements in one line as it is a PITA 
to debug.

>
> The problem is that it changes the API, thus requiring a lot of work
> to migrate tools using Ivy 1.4 API. That's why I've added a Ivy14
> class, which provide a 1.4 compatible API, so that if you don't want
> to use the new API, you can simply replace your instance of Ivy by an
> instance of Ivy14, and you're done!

>
> So, what do you think? Shall I continue on this way, and do that for
> all main methods of Ivy? Or go back to the 1.4 API?

Simplification and partitioning is good. :)

I don't have much time to review it until the week end though.

-- stephane