james-mime4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oleg Kalnichevski <ol...@apache.org>
Subject Re: [cycleclean] branch review and questions
Date Thu, 07 Jan 2010 17:17:37 GMT
Stefano Bagnara wrote:
> 2010/1/7 Oleg Kalnichevski <olegk@apache.org>:
>>> All I need is to have a dependency free token stream parser. If you
>>> can suggest a better option that extracting the BaseMimeTokenStream
>>> abstract class I'm fine with it. While I may like introducing an
>>> interface, I fail to see how it fixes any issue here. Can you go in
>>> details? (I don't understand why the abstract class hurts you so
>>> much..)
>> Because it is not abstract to start with and is called Basic* not Base*.
>> I simply do not understand the distinction between the two classes from
>> the functional standpoint. Why one concrete class has been thrown into a
>> public api package and another one into the impl package. This just does
>> not sound right.
> 
> If you have any better name you're welcome! I usually don't care too
> much about names at this step of my refactorings. So I agree that most
> class names and most package names can be fixed (I tried to change as
> few as I could to help reviewing, and when I introduced new names I
> chose the one that made sense to me at that moment, nothing written in
> stone).
> 
> The Basic parser is a parser that know nothing about field parser
> implementations, it simply contains the basic logic. You can use it if
> you need your own BodyDescriptor and the 2 basic implementations are
> not enough for you, or if you want to use the parser without the whole
> field dependency.
> 
> About parser vs parser.impl I agree with you. parser.impl could be
> renamed to parser and parser could be moved to parser.stream or
> something similar. This way the entry points remain in the same
> package. Also I don't like to much "impl" as a package name. I used
> impl everywhere because this meant moving less code than doing
> something else. E.g: I moved message implementation from message to
> message.impl, but maybe a better approach can be to rename "message"
> to "dom" and put back "message.impl" in "message". Same for field. my
> "field" could be moved to "dom.field" and then "field.impl" moved back
> to "field".  I'm really open to this, it's just I thought I would have
> introduced more changes to your eyes. If you think it worth doing this
> move I'll do this in the branch so you can see it applied.
> 
>> I will work on an alternative approach in the coming days
> 
> Cool!
> 
>>>> (2) Please make sure  LineReaderInputStream#unread() does not impose a
>>>> particular mode of operation.
>>> This is easy to change. I still think this is worse (performance and
>>> memory wise) than the current solution. It is a really internal class
>>> that should not be used by users and, in the last revision, it already
>>> protects from multiple unread calls using exceptions. We use the
>>> "unread" method in a single place of our code (furthermore this
>>> already exists and works, other solutions should be implemented). If
>>> we don't want to optimize it at the extreme maybe we can switch the
>>> whole line reading behaviour to a Buffered Reader, so that we can
>>> simply use mark/reset. But even mark/resent impose a particular mode
>>> of operation. The fact is that we talk about buffers. If you don't
>>> want to impose limits then you need infinite memory :-)
>>>
>> See my main issue above.
> 
> Thank you :-)
> 
> Stefano
> 

If there are no major objection, I could volunteer to work on merging 
down three low level packages: utils, io, and parser from the Stefano's 
branch and also try to resolve the MimeTokenStream issue.

Oleg

Mime
View raw message