[Aptitude-devel] Stepping down as maintainer/developer

[Manuel A. Fernandez Montecelo]

2012/2/28 Daniel Hartwig <mandyke at gmail.com>:
> Upon looking at the changes there were some points which I felt were a
> technical concern, some style issues, and some other minor questions.
> I had hoped to convey that--even though I was raising these
> concerns--the changes were still functional, appeared correct, and
> were fine for inclusion.

So what's technically wrong with this one:
http://anonscm.debian.org/gitweb/?p=aptitude/aptitude.git;a=commitdiff;h=c6e2914b0c751d591ad3e39743c7d417e7f6aaef

That you changed to:
http://anonscm.debian.org/gitweb/?p=aptitude/aptitude.git;a=commitdiff;h=69deef1f4a9678a67a547b5dd8c35f6303832ce0

>From http://en.wikipedia.org/wiki/Refactoring
"[Overview]
Refactoring is usually motivated by noticing a code smell.[3] For
example the method at hand may be very long, or it may be a near
duplicate of another nearby method."

So one of my methods, which is functionally equivalent to the other,
calls it the other with the right parameters [1].  This avois
repetition of code, which is Good(TM), and instead you replace it with
Copy&Pasting(TM) code, which is universally understood as contrary to
a good practice.

Would you mind explaining why "Make FindVector consistent with other
wrappers" (the commit message) justifies this?  Is it just to follow
the [anti]pattern of the rest of the file?  To abide to the legacy?

Shouldn't the rest of the methods be "fixed" instead of mine?

[1] A call "method(const char* str)" will fall back to "method(string
str)" if the first one is missing, as far as I know, so even this
might be unnecessary.


Now, onto refactoring.  "Martin Fowler's book Refactoring: Improving
the Design of Existing Code[3] is the canonical reference."

"When should you refactor? (chapter 2 page 57)"

"When I talk about refactoring, I'm often asked about how it should be
scheduled.  Should we allocate two weeks every couple of months to
refactoring?

In almost all cases, I'm opposed to setting aside time for
refactoring.  In my view refactoring is not an activity you set aside
time to do.  Refactoring is something you do all the time in little
bursts.  You don't decide to refactor, you refactor because you want
to do somethign else, and refactoring helps you to do that other
thing."

So for me, changing "string" to "const string&" is refactoring, and
should be done along the way, not to set aside time for that (or a
commit of two lines).  Because there's never time scheduled for such
small things, and you almost always spot "refactoring opportunities"
when you're knee-deep in other changes, you either fix it at that
point or don't bother.

Same for the includes, same for the additional blank lines after the
headers (which was a mistake oversight, but which I was surprised that
somebody would bother to mention that at all in a review).

Cleaning up the whole of load_sortpolicy.cc, using better variable
names and so on, is another candidate for refactoring.  That you find
extreamely readable the name "rval" and that I pissed you off is one
thing, but it's not obvious for everybody, or not everybody finds it a
particularly good name.

So when you complain about why I do change things and that I'm
breaking legacy, well, that's often a bad thing to do when the old
group of developers is present, to not disrupt things.  When people
come afresh into a [somewhat] dead project, revisiting old decisions
and practices (part of refactoring) usually helps to improve overall
quality, but most importantly make the new people more intimately know
the code and better prepared to fix the present and upcoming problems.
 And helps to establish the "new legacy", instead of having to cope
forever with the legacy established by others and that the new ones
necessarily do not agree with.

It was my fault to be too eager on changing some things without
asking, but it looks to me that you're completely against all what I
did, revisiting all what I did, both my "sound" (IMO) changes with
wrong fixes and the little bits with the legacy, giving the impression
that you don't want to change anything at all.

Well, so be it, it's all yours.

Cheers.