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: [cycleclean] branch review and questions
Date Thu, 07 Jan 2010 15:45:33 GMT
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

Mime
View raw message