code quality is important, and there are things we should keep in
mind, but in general i really don't like the idea of risking code
breakage because of a gratuitous code cleanup. we should be watching out
for these things when patches get submitted or when new things go in.
i think this is inline with what pat was saying. just to expand a bit.
in my opinion clean up refactorings have the following problems:
1) you risk breaking things in production for a potential future
maintenance advantage.
2) there is always subjectivity: quality code for one code quality
zealot is often seen as a bad design by another code quality zealot.
unless there is an objective reason to do it, don't.
3) you may cleanup the wrong way. you may restructure to make the
current code clean and then end up rewriting and refactoring again to
change the logic.
i think we can mitigate 1) by only doing it when necessary. as a
corollary we can mitigate 2) and 3) by only doing refactoring/cleanups
when motivated by some new change: fix a bug, increased performance, new
feature, etc.
ben
On 10/13/2010 06:18 AM, Thomas Koch wrote:
Hi,
after filling 13 refactoring issues against the Java Client code[1], I started
to dig into the server site code to understand the last issues with the Netty
stuff.
I feel bad. It's this feeling of "I don't wanna hurt you, but...". ZooKeeper
is quite an important piece of the Hadoop ecosystem containing some of the
most complicated pieces of code. And it'll only get more complex with more
features.
I'd propose to have a word about quality assurance. Is there already a
strategy to ensure the ongoing maintainability of ZK? Is there a code style
guide, a list of Dos-And-Donts (where I'd like to add some points)?
Should PMD be added to Hudson? What is the level of FindBugs? Should it be
raised?
Some of the points I'd like to add to a style guide:
- Don't write methods longer then 20-40 lines of code
- Are you sure you want to use inner classes?
- If there is a new operator in a method? Could the method maybe already
receive the object as a parameter?
- Are you sure you want to use system properties? They are like global
variables and the IDE does not know about them
- Are you sure you want to extend a class? Often an aggregation is more
elegant.
- Don't nest ifs and loops deeper then 2 or 3 levels. If you do so, you should
better break your code into more methods.
- Use Enums or constants instead of plain status integers
- please document your intentions in code comments. You don't need to comment
the what? but the why?.
Do you agree with me, that there is a need for better code quality in
ZooKeeper? If so, it's not really scalable if a manic like me fights like Don
Quichotte to clean up the code. All developers would need to establish a sense
for clean code and constantly improve the code.
[1] https://issues.apache.org/jira/browse/ZOOKEEPER-835
Best regards,
Thomas Koch, http://www.koch.ro
