2012/2/28 Daniel Hartwig <[email protected]>: > 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. _______________________________________________ Aptitude-devel mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/aptitude-devel
