[jira] Commented: (JAMES-509) Cleanup/Refactor FetchMail code

["Steve Brewin (JIRA)" <se...@james.apache.org>]

    [ http://issues.apache.org/jira/browse/JAMES-509?page=comments#action_12413314 ] 

Steve Brewin commented on JAMES-509:
------------------------------------

For me, this is a prime example of why people have said we need to discuss first, achieve consensus, then act.

I'm afraid I don't understand why this wasn't discussed first on the lists. Is it something we need to do? What is the motivation for such a major refactoring? We all know that major changes are prone to creating new issues. What functional issues are being solved? Are they of sufficient importance to outweigh the risk of destabilisation such a radical change engenders? What additional features are provided?

The argument for change seems to be "Current FetchMail code is really hard to read and manage". Others have commented how well structured it is (I don't have time to look up the references). This is a matter of style. I'm biased as I refactored the original code into the structure you see now and added a heap of functionally. I don't want to get into an argument about style or how virtuous having "removed 2200 lines of code (50Kbytes of code)" is.

I think that the changes are so radical that we are in effect replacing the current implementation of fetchmail and need to ask why? Unlike when I refactored it, it isn't broken and no new features are added so we are not satisfying any user requirements. 

In making these changes, without even a safety net of unit tests, we are destabilising a component for no more than a matter of style. To me, this doesn't seem wise. There are no obvious benefits that outweigh the risks.

-- Steve

> Cleanup/Refactor FetchMail code
> -------------------------------
>
>          Key: JAMES-509
>          URL: http://issues.apache.org/jira/browse/JAMES-509
>      Project: James
>         Type: Improvement

>   Components: FetchMail
>     Versions: 2.4.0
>     Reporter: Stefano Bagnara
>     Assignee: Stefano Bagnara
>      Fix For: 2.4.0
>  Attachments: fetchmail-refactoring1
>
> Current FetchMail code is really hard to read and manage.
> I loose too much time looking around its code to understand how things works.
> So I put my hands in, and applied a few refactoring.
> I already removed 2200 lines of code (50Kbytes of code) while keeping the same functionality (only refactorings).
> Of course refactorings like this are not always an easy change: I also remove the StoreProcessor/FolderProcessor/MessageProcessor granular creation by refactoring them to top level reentrant objects that take things to process as arguments to their "process" method.
> MessageProcessor is still a mess, but I think this is more selfdocumenting than before.
> I can clean up things much more and update documentation and so on, but I would like to know if this kind of update is welcome or not, before loosing too much time.
> Stefano

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org

Re: [jira] Commented: (JAMES-509) Cleanup/Refactor FetchMail code

[Joachim Draeger <jd...@joachim-draeger.de>]

Steve Brewin (JIRA) schrieb:

> For me, this is a prime example of why people have said we need to discuss first, achieve consensus, then act.

There is a patch, that we can discuss. If it makes sense it should be 
included.

> I'm afraid I don't understand why this wasn't discussed first on the lists. Is it something we need to do? What is the motivation for such a major refactoring? We all know that major changes are prone to creating new issues. What functional issues are being solved? Are they of sufficient importance to outweigh the risk of destabilisation such a radical change engenders? What additional features are provided?
> 
> The argument for change seems to be "Current FetchMail code is really hard to read and manage". Others have commented how well structured it is (I don't have time to look up the references). This is a matter of style. I'm biased as I refactored the original code into the structure you see now and added a heap of functionally. I don't want to get into an argument about style or how virtuous having "removed 2200 lines of code (50Kbytes of code)" is.
> 
> I think that the changes are so radical that we are in effect replacing the current implementation of fetchmail and need to ask why? Unlike when I refactored it, it isn't broken and no new features are added so we are not satisfying any user requirements. 
> 
> In making these changes, without even a safety net of unit tests, we are destabilising a component for no more than a matter of style. To me, this doesn't seem wise. There are no obvious benefits that outweigh the risks.

I think saving time is a very serious argument. This is my own 
experience: Sometimes there are components which aren't subject of big 
changes but you always have to go through of them and maybe make small 
changes. And when the code is hard to read and hard to understand it 
costs a lot of time. At the end you could have rewrite it from scratch 
and you would have saved much time and that means a lot of money in the 
commercial world. Sometimes I do this step and refactor/rewrite and 
sometimes I am annoyed that I haven't done it. Yes it can be reasonable 
touching code without bringing features for the end user.
Of course there are risks and it has to be discussed that things don't 
get worse. So demanding unit tests maybe an argument. On the other hand 
keeping hard to manage code is a risk, too, because it is error-prone 
when you have to do changes.
Time isn't money in the open source world although time of volunteers it 
the most valuably. We should not waste it.

I haven't had my hands in the fetchmail code so I only can do a fast 
review. And the patch makes sense to me. For example there are getters 
an setters for variables only used locally in one method. If you don't 
have them local you always have to check for references and side effects 
when reading that code. IMHO it looks a bit over-designed to me.


Joachim

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org