Re: Proposed API Refactoring

[Stefan Bodewig <bo...@apache.org>]

I thought I'd better give a late feedback than none.

Simeon Fitch <me...@yahoo.com> wrote:

> B) The Project.java file also has a lot of fungal growth :-P. It's
>    "init()" method (which is is separate from the constructor, I
>    assume for BuildEvent reasons) executes the following tasks:

Actually I think we've made it that way to help <ant> and <antcall>.

>    2) Loads the default properties from default.properties.

These are not the default properties but the mapping of task names to
implementation classes, but you knew that 8-)

> C) Here is /a/ proposed refactoring:
>    1) Create a class called "Args" that is responsible for:

+1

>    2) Create a "Logger" class. It would be initialized with the
>    return values of Args.getStdOut(), Args.getStdErr(),
>    Args.getLogLevel(), and probably some other log listener
>    method.

Initialized by Main or whom? We should tie the Logger to Args as the
latter will only be used in command line mode while a Logger will be
needed anyway.

>    3) The logging level "static final int" variables (MSG_WARN,
>       MSG_INFO, etc.) belong either in BuildEvent, the Proposed
>       "Logger" class, or in a separate enumeration class.

I'm no big friend of enumeration classes, my preference would be
inside Logger.

>    4) Create a "AntProperties" class, which takes care of loading an
>       overriding properties based on the default.properties, command
>       line properties and .ant.properties data sets.

Not sure about that, maybe an approach of having a PropertyStore that
unifies <property> with references (making <property> just the String
version of a data type) might be the right place to add this
responsibilities.

>    5) Create a "DataTypeManager" that is responsible for loading and
>       initializing the data types based on the property sets.

Could by used for Task creations as well as data type creation, some
kind of Factory.

>    6) Better encapsulation of the event handling. [...] there should
>    be a *single* place for registering BuildListener instances and
>    firing events.
> 
>       I think the Project class is still a good place to have the
>       add/remove listener methods

Agreed.

>       but access to the event firing code needs to be opened up,

Why? Who should be firing what type of events?

>    7) Relocation of XML parser initialization to the
>       ProjectHelper.

Yes.

>    8) The need for separating the initialization code in the Project
>       class into an "init()" method instead of in the constructor
>       can be eliminated

If just reread Ant.java. 

<ant> needs a Project instance at parser time as it uses this one for
the nested <property> elements. Project objects are quite heavy ATM
(speaking of memory consumption) and basically duplicate Hashtables
from the parent project.

What <ant> does right now is to instantiate a minimalistic Project
instance that doesn't know much, it can only create Property tasks and
knows the Java version, that's all. As soon as the <ant> task gets
executed, the Project.init will be called - and the whole project
instance will be left to the garbage collector at the end of execute.

If you have many <ant> tasks in your build file, this saves tons of
memory as opposed to having many full blown Project instances from
parser time to Ant termination.

Following all your proposals Project might end up being quite small,
so this issue might go away.

>    9) [...] moving the
>       "fireBuild{Started|Finished}()" invocation from Main.java into
>       the "executeTarget()" method.

Not sure whether this is the right place but Project should be the one
calling this methods - no question.

>    10) Either colocate the file based utility methods (fileCopy(),
>        getParentFile()),

I'd like to see some stuff like copying of files and filter
replacements to separate classes anyway. Copying of files could be
extended in an operating system dependant manner (copying the resource
fork on Macs for example) a lot easier if it was placed into a class
of its own.

>    11) Create a "TargetCollection" class that manages a set of
>        targets, and knows how to sort them in "target" specific
>        ways.

Agreed.

>    12) There has been a discussion lately about a more general
>        token replacement facility. If this comes to fruition,
>        hopefully the code to do this will be moved out of the
>        Project class.

Yes, yes, yes.

>    14) The API needs to be tuned up for reentrancy and thread
>        safty.

Probably - given that we want to open Ant to a whole bunch of new
frontends (servlets, incremental mode Ants ...) we shouldn't rely on
running a single threaded application.

>    100) Javadoc, javadoc, javadoc.

8-)

Stefan