james-mime4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefano Bagnara <apa...@bago.org>
Subject Re: MessageBuilder refactoring; was Re: dom / message refactoring
Date Wed, 26 Jan 2011 22:21:22 GMT
2011/1/26 Oleg Kalnichevski <olegk@apache.org>:
> [...]
> Fine. The problem is that Service Locator is considered an anti pattern
> by many, an opinion I fully subscribe to.

I don't have a strong opinion on this. IMHO it depends on the context
(also, an antipattern is usually compared to a better alternative to
reach the same goal).

If you want to remove it from svn feel free to do that. I just care
about the operational fixes/improvements I did there and don't care
about the locator or the dom api (sooner or later it would be good to
have this api, but it is not an high priority IMO).

Consider it a test. I did it in trunk simply because no one else was
working there and we usually prefer coding to happen in trunk (in fact
the locator+factories is something that took few hours, nothing more
than an experiment and we probably wasting too much time "discussing"
it when it can either be improved or replaced by alternatives)

I'm not against removing the locator, removing the "abstract classes
style" and so on (and you should feel free doing this, really).

The main "design" thing I care is about package dependencies: I don't
want dependency cycles between packages (I know some developers don't
care about this, but in my experience it is very important and I try
to defend this as I can: I'm aware people think I'm extremist about
this).

> [...]
> What is wrong with plain old java classes? What is it _exactly_ that was
> wrong with Message classes in 0.6?

goals in my mind when I did the change:
1) having a single package exposing dom functionality/classes so
client classes simply have "import mime4j.dom.*;" and not some import
from parser some from stream some from message and some from field.
2) make sure this package had no direct dependencies on the parsing
package (DOM should not depend on a parsing implementation: DOM is
about structure, not parsing)
3) allow future release to be able to add new methods/behaviours
without breaking compatibilty.

I remember 0.6 had many other issue I had to refactor in order to come
up with clean dependency tree and fix some bug regarding consistency
(headless parsing, header folding, empty lines), but if you want to
backport fixes and keep the "Message" style from 0.6 I can live with
it too. I'm sorry but my memory don't let me remember all of the issue
I saw in 0.6 (I don't even remember what I ate yesterday). Another
motivation was using an API java developers were used to (Most java
developers used/understand xml api so it was a good match).

> I am not sure. Now we have a situation when neither you nor I like the
> existing DOM API that much.

If I understand the way you liked the 0.6 version then it should be
simple to make you like trunk again.
1) Just leave interfaces in dom.
2) Remove the factories and service locator from dom (or ignore they
exists, they are just something "additional" you are not forced to use
them).
3) Tell people to instantiate Message implementation from message
package (MimeMessage / MessageImpl).

How is so much different from 0.6 with dom interfaces extracted to
another package?
Are you instead saying you don't want the "dom" package at all?

While I try to remember something else from the upgrade of my "client
code" from 0.6 to 0.7-SNAPSHOT I personally prefer the way some
configuration is exposed via the new factories instead of the 0.6
method of passing a MimeEntityConfig whenever you wanted to alter the
default behaviour (I think config objects like this are unexpected
from a java library user).

jDKIM with mime4j 0.6:
http://svn.apache.org/viewvc/james/jdkim/trunk/main/src/main/java/org/apache/james/jdkim/impl/Message.java?revision=824254&view=markup
You can see I had to extend MimeTokenStream  just to be able to
configure a new max line length.
Also I had to use the low level token stream for a simple task like
parsing message headers.

jDKIM with mime4j 0.7-SNAP (didn't update to the current trunk, it is
still against trunk before you worked on it):
http://svn.apache.org/viewvc/james/jdkim/trunk/main/src/main/java/org/apache/james/jdkim/impl/Message.java?revision=998320&view=markup
It still uses MimeEntityConfig but you can see the comment where I was
planning to have similar configurations exposed as factory properties:
--
MessageBuilderFactory mbf = MessageBuilderFactory.newInstance();
68 	mbf.setAttribute("MimeEntityConfig", mec);
69 	// mbf.setProperty("MaxLineLength", 10000);
---
Once MaxLineLength was a factory configuration property the jDKIM
would be using only dom classes (and not message/parser/stream lower
level packages).

Also the code moved from 184 to 130 LOC.

Unfortunately my experience with mime4j <=0.6 is limited to jdkim
because the only other project where I used mime4j used 0.7-SNAP since
the first version because I use the validation and the Monitor to
report errors found while parsing (not available in 0.6).

> Anyway, I'll stick to my original plan, bring mime4j to a state in which
> it could be released as 0.7 and step back again.

OK (and feel free to drop my trunk tests if you have ideas, really)

Stefano

PS: This long messages are not to "defend" my opinion, but just to
explain trunk code and historical steps.

Mime
View raw message