I broadly agree with Ben - all meaningful code changes carry a risk of destabilization (otherwise software development would be very easy) so we should guard against improving cleanliness only for its own sake. At the point where bad code gets in the way of fixing bugs or adding features, I think it's very worthwhile to 'lazily' clean code.
I did this with the observers patch - reworked some of the class hierarchies to improve encapsulation and make it easier to add new implementations. The netty patch is a good test case for this approach. If we feel that reworking the structure of the existing server cnxn code will make it significantly easier to add a second implementation that adheres to the same interface, then I say that such a refactoring is worthwhile, but even then only if it's straightforward to make the changes while convincing ourselves that the behaviour of the new implementation is consistent with the old. Thomas, do comment on the patch itself! That's the very best way to make sure your concerns get heard and addressed. cheers, Henry On 15 October 2010 11:37, Benjamin Reed <br...@yahoo-inc.com> wrote: > i think we have a very different perspective on the quality issue: > > > > I didn't want to say it that clear, but especially the new Netty code, >> both on client and server side is IMHO an example of new code in very bad >> shape. The >> client code patch even changes the FindBugs configuration to exclude the >> new >> code from the FindBugs checks. >> >> great. fixing the code and refactoring before a patch goes in is the > perfect time to do it! please give feedback and help make the patch better. > there is a reason to exclude checks (which is why there is such excludes), > but if we can avoid them we should. before a patch is applied is exactly the > time to do cleanup > > If your code is already in such a bad shape, that every change includes >> considerable risk to break something, then you already are in trouble. >> With >> every new feature (or bugfix!) you also risk to break something. >> If you don't have the attitude of permanent refactoring to improve the >> code >> quality, you will inevitably lower the maintainability of your code with >> every >> new feature. New features will build on the dirty concepts already in the >> code >> and therfor make it more expensive to ever clean things up. >> > cleaning up code to add a new feature is a great time to clean up the code. > > Yes. Refactoring isn't easy, but necessary. Only over time you better >> understand your domain and find better structures. Over time you introduce >> features that let code grow so that it should better be split up in >> smaller >> units that the human brain can still handle. >> >> it is the "but necessary" that i disagree with. there is plenty of code > that could be cleaned up and made to look a lot nicer, but we shouldn't > touch it, unless we are fixing something else or adding a new feature. it's > pretty lame to explain to someone that the bug that was introduced by a code > change was motivated by a desire to make the code cleaner. any code change > runs the risk of breakage, thus changing code simply for cleanliness is not > worth the risk. > > ben > -- Henry Robinson Software Engineer Cloudera 415-994-6679
