On Tue, Apr 7, 2009 at 10:11 PM, Avinash Lakshman <avinash.laksh...@gmail.com> wrote: > The part that is very disconcerting are the following: > (1) If one becomes a committer one is not expected to blitz through the code > base and start refactoring everything.
There are two reasons I refactor. One is, I always try to leave the code better than I found it. This helps fight technical debt. In a project the size of cassandra you have to do this or you end up with fragile code that you cannot change without introducing regressions. Usually this is not because the original code was bad, but because the requirements changed as the project grew and now there is a better way. Or sometimes there is an idiom better suited to a situation that the original author was not familiar with. That's okay too; nobody's perfect. An example of this is r762440, replace String.indexOf != -1 with String.contains (review by johano). Two is, if I am going to introduce a new feature I will try to refactor first without changing behavior in such away that the feature becomes easier to add. This breaks the changes up into smaller pieces which are easier to review and easier to validate against regressions. (The more things you change at once, the harder it is to find which caused a problem.) A specific class of this kind of change is merging copy/pasted code into method calls. Duplicate code makes it hard to add features because you have to know about the duplicates and remember change all when you change one. There is a lot of this in cassandra. Examples include r762440, r762440, r762440. (This specific area of the code did bite me while implementing remove; that's what the "os x error" thread on google groups was about.) So there is a method to my madness. :) > In any organization one doesn't just go about ripping out everyone > else's code for no rhyme or reason. That will offend anybody. I personally > would not go about ripping someone else's code apart if I had become > committer. It is just that respect ought to be there. I think there may be cultural differences here. It is not a sign of disrespect to think that I can improve your code or anyone's. :) > (2) This is something that I have said many times over. Certain things are > the way they are for a reason. And others are not. For instance, there's a lot of classes that still implement Serializable, but that's misleading because you roll your own serialization now. Or for something more meaningful, the code-by-copy-paste examples. It's just not reasonable to say "don't touch anything, it's all done for a reason" when there are clear counterexamples right there in the code. I don't see any alternative to your becoming involved in the review process so you can tell us when something really is important, such as > For example when I say ConcurrentHashMap is a > memory hog I say it because we have seen this in practice. How does it > manifest itself? I obviously do not recall since all this was over a year > ago. So I replaced those with NonBlockingHashMap. The system is working. :) > No one can claim to have run tests the way we have in the last year and > a half. One cannot just run some simple test and say well I do not see the > problem. I am not dumb. Again, maybe there are cultural differences here... Todd was asking out of curiosity and a desire for more information. Often knowing how you arrived at a conclusion is as useful or more so than the conclusion itself. (Otherwise you run the risk of encouraging cargo cult programming.) > My understanding was that new committers come in and start with some feature > implement that and then slowly start looking into what more they could do > going forward. That's what happened... in December. I've had a lot of time to look. :) > It is NOT come in and refactor the hell out of the system > because you like something to be in a specific way. I think I addressed this above, but I'm happy to discuss specific commits in more detail. -Jonathan
