Document has been moved to Smack’s Github Wiki: https://github.com/igniterealtime/Smack/wiki/Guidelines-for-Smack-Developers-and -Contributors
1 Like
Hi,
this is a first draft for a Guideline for Smack Contributors.
Guenther Niess already took a look at it and mentioned that formatting of if/else statements used in the current Smack source differs from the Java Conventions.
if (condition) {
// do something
} else {
// do something different
}
vs.
if (condition) {
// do something
}
else {
// do something different
}
I changed that and did a little code statistic to prove it:
- else {
grep -r -e "^ *else {" --include="*.java" . | wc -l
=> 167
- } else {
grep -r -e "^ *} else {" --include="*.java" . | wc -l
=> 32
Same goes for try/catch/finally:
- } catch ...
grep -r -e "^ *} catch" --include="*.java" . | wc -l
=> 32
- catch ...
grep -r -e "^ *catch" --include="*.java" . | wc -l
=> 190
Also statistics prove that a block should be opened in the same line as the statement
public void test() {
// do something
}
- block starting next line after declaration
grep -r -e ")$" --include="*.java" . | wc -l
=> 265
- block starting at same line as declaration
grep -r -e ") {$" --include="*.java" . | wc -l
=> 3670
Feel free to discuss and improve the document.
Maybe we could extract the parts that only concern Smack and use the guidelines for other Projects too.
Best regards,
Henning
Hi Henning,
This is an excellent initiative! Thanks for taking the time to formalize things. This will help maintaining or even improve on code quality of Smack. I reviewed your text, and agree to most of what you write. I do have a couple of remarks, which you’ll find below.
I’m not particularly in favor of enforcing very strict formatting rules. I’m ok with “guidelines,” as opposed to “rules.” We should not scare people off - one should be able to follow a style that he/she is comfortable with. I can think of only two things related to formatting that I would like to see enforced:
- existing code must remain in the same formatting style! In other words: you must not refactor a class just to make the code style match a particular style. This will make it near impossible to do code comparison with previous versions of the code - something that can be essential while tracking down a regressed bug.
- every code block (in if or while statements) must be enclosed by { }. By not doing this, one relies on the code layout (tab) for correctness. Bugs are easily introduced this way. In other words: do “if (condition) { statement; }” instead of “if (condition) statement;” (I do agree that statements should go on a new line).
I disagree strongly with the proposed exception handling. I feel that we should not wrap any and every exception in an XMPPException. This should be done only where this makes sense. Note that the ‘catch (Exception ex)’ statement as used in the example will not only catch all checked exceptions, but also all non-checked/runtime exceptions. In most cases, this is bad. Runtime errors (IllegalArgumentException, NullPointerException, etc, etc) typically indicate problems in the application code (as opposed to its usage). They should not be hidden - instead, simply allow them to bubble up. Additionally, it might not be appropiate to use the same class for every exception. It should not be a requirement to use XMPPException only.
Please include a section on thread safety! Although concurrency-related issues can be very complex, we can (and should!) apply simple techniques to reduce the likelyhood of concurrency related bugs to pop up. From the top of my head, I can think of these:
- Prefer immutable objects. Immutable objects are by definition thread-safe. Use the final keyword for every field this is not changed other than by the constructor.
- Document concurrency related behavior. It should be clear to others if a particular class is (intended to be/remain) thread safe, or not). For the Tinder project, I have started to use the JCIP-based annotations to do this. Apart from documenting the code, static analyzers (such as FindBugs) can make use these annotations, allowing for better analysis of the code.
That’s all what I could think of while reading your document. Perhaps we could apply this to other projects too (or have a “general” guideline document with extensions for individual projects). In any case, good work!
Hi,
I totally agree with Guus and vote that this document should be for all ignite or at least for all Java projects. I think it would be nice to have such a code style guidline
Hi,
thanks for the review.
I agree that all statements within an condition or a loop should be surrounded by braces and edited the guideline (section Braces) to reflect this.
I also edited the exception handling section. It was not my intension to force people to wrap everything in XMPPException. The only thing I wanted to say is “don’t concat exception messages, instead wrap them.”. There are some occurences in the code where this is done wrong.
I also added a Thread Safety section but besides the things you mentioned I couldn’t think of any other points because I’m not very familiar with this topic.
I partially agree with “existing code must remain in the same formatting style”. In a perfect world all code of a project should be formatted the same way with rules that can be applied automatically. So if anybody adds code to a file he can just auto-format and it is assured that the changes don’t contain any unnecessary formatting. That’s why I attached the Eclipse formatting configuration.
Besides regressions should be prevented by unit tests.
Hi,
just added a point to the packages section that providers should also be located within the package of a feature analogue to the packets.
What do you think?
I agree with Guus on several of his points as well. Of course if you do, it really means gutting a large section of this document dealing with formatting, not simply using it for all projects.
I have wasted too much time in meetings over the past years (thankfully it has been a few now) over disagreements on how code should be formatted. Too many “rules” or “guidelines” will always be viewed as subjective and typically nothing more than an attempt to enforce ones views/opionions on the masses. Frankly, I couldn’t care less if the curly brace is on the same line or the following one (I prefer with the following line). There are only a couple of formatting points that I agree with, most I think should be deleted. Basically I think that the section on Braces, Blank Lines, New Lines, Comments and Control Flow should be scratched (sorry, I don’t agree with Guus on that one). The Line Wrapping is OK, but 80 charactes is much too short in this day and age. Frankly if you are doing development on a terminal server, it is time to change your environment. No need for the rest of us to suffer for that shortcoming. I would say 100-120 chars would be appropriate.
The other problem with trying to set all these rules is that it will encourage even more rules. I don’t want to see disagreements over code being acceptable based on things like usage of final for variable declarations and method parameters. Have a look for such things on stackoverflow to see the differing points of view on that one.
Also, Guus’s point on reformatting existing code cannot be taken lightly. It is critical to maintain a useful history of the code. This fact automatically means that you don’t use an autoformatting utility like the one provided for Eclipse.
Now I don’t want to seem like I am disagreeing with everything. In fact, the other sections have some really good stuff or make good starting points (like Threading) in there and I believe make excellent guidelines that are very relevant to the codebase in question.
I added a sentence in the introduction of the code styleguide that code contributions will not be declined if they don’t match the styleguide.
So developers that like to have a styleguide (like me) have something they can rely on and developers who think this is unnecessary don’t have to bother about it.
I also increased the maximum chars per line to 100. I totally agree with you that 80 chars is not appropriate in modern development environments anymore.
Also, Guus’s point on reformatting existing code cannot be taken lightly. It is critical to maintain a useful history of the code. This fact automatically means that you don’t use an autoformatting utility like the one provided for Eclipse…
I really don’t get that point. The history of the code is maintained by SVN repository. If you hunt down a bug from a specific smack version (with a stacktrace) you will always have to look at the source file in the same revision as the one where the stack trace comes from. How could reformatting the code make this any more difficult?
Autoformatting is a really nice feature that can save time because you only have to concentrate on the code you write and don’t have to think about line breaks and stuff.
I don’t think that this document should be used for all Java based projects for now. Instead we should publish this one just for Smack and see how developers are reacting. If it’s ignored by the people we don’t have to make an effort to generalize the document because it will be ignored as well. If not the document will be refined over the time and then we can extract a generalized version that could be used for other projects as well.
The problem is not the existence of the history, its the ability to compare what changes have been made between versions. If the file has been reformatted, then the ability to see what has changed becomes severely impaired (if not impossible) due to it being hidden by the differences caused by reformatting.
Take a worse case example. I have a file with only 3 spaces used for indenting and I reformat it to 4. Now there is practically no way for most diff tools to track the changes that occurred before that change and after since every line of code will show up as changed. Any comparisons would have to be done manually over the entire file.
The biggest problem with this approach is that Smack seems to have a very limited contributor base right now, so it may simply not get exercised.
As nearly no one commits to Smack it should be no problem to apply the new code style to Smack. If one is really interested in changes which are not related to the re-formatting one can still chekout an old version, re-format and then diff it.
Same applies to Openfire, one could make a 3.4.0f version with re-formated code as 3.4.0 is now quite old/stable.
This assumes that everyone is using the same code style settings. If anyone uses one that is different then this strategy won’t work. It also won’t work as the code style changes over time, as it inevitably will. I would make some changes myself as my preferences are different from those that were documented. I mean, they are at this point simply one persons personal preferences and certainly don’t necesarily reflect the preferences of others, I know they don’t reflect mine (not entirely) and I have been one of the biggest contributors to Smack in the last 8 months.
That is why I suggest that any code formatting should only be applied against edited code, not the entire file. This is much safer and will allow anyone to achieve the format they like in the code that they are actually working on, without the negative impacts of changing the whole file.
If it ain’t broken, don’t fix it.
By now I agree to Robins opinion that a code style includes personal preferences and it will change over time. So it is not necessary to reformat all existing code. This would be possible in a company but is not applicable in an open source project. Having developers contributing code in their own style is more important than having consitent styled source code.
Nevertheless having such a styleguide is still a good thing for all developers who don’t have personal preferences or who like to see their own code be consistent with the bigger part of the existing code.
1 Like