Home » forums » General » General discussions

JNode needs a style guide

Submitted by Stephen Crawley on Wed, 04/23/2008 - 11:34. General discussions

I've seen quite a bit of the JNode code base, and one thing that I find irritating is the inconsistent and occasionally bad coding style. I see:

  • inconsistent indentation,
  • inconsistent use of whitespace around operators, before and after braces, parentheses, etc
  • inconsistent spacing between lines,
  • files containing TAB characters,
  • minimal useful in-line comments
  • useless or non-existing Javadoc comments on key methods, or entire APIs.

I've also come across some basic coding errors that would have been picked up by a cursory code review. I've seen command classes that catch java.lang.Exception, command classes (with 'execute' methods) that write to System.out / System.err rather than the command's out/err, code that repeatedly looks up the same system service in loops. I've seen left-over trace prints, and Unsafe.debug calls. I've seen code that is so hacked around that it is unintelligible. And some of codebase's the lack of concern for multi-threaded access is truly frightening!

I think it is time that we formally set out some JNode code style guidelines ... and start paying attention to the quality of code that gets checked in.

For the record, I've been developing code in many languages and many contexts for over 30 years now. In my experience, consistent code style is very important to the long-term maintainability of a code-base. While getting the code style right can be a chore, it is worth the effort in the long term. Besides, an IDE like Eclipse or IntelliJ (or even good old Emacs) can do a reasonable job of fixing up your code's indentation.

» login or register to post comments | previous forum topic | next forum topic

using QALab

Submitted by Fabien D on Wed, 04/23/2008 - 18:39.

I think we should even use QALab (http://qalab.sourceforge.net), which measure the evolution of overall quality of the source code. Among other, it is using pmd and checkstyle. I will try to setup that locally on my PC.

Of course, we still have to decide of the rules to apply but that will allow us to be sure we are going in the right way in term of code quality.

Fabien

my blog

» login or register to post comments

I like your idea too, we

Submitted by Peter on Wed, 04/23/2008 - 17:42.

I like your idea too, we had already a discussion and probably a doc for that (Martin perhaps still has it and searches for it) which was considered as a discussion base. Anyway I like what levente said:

- indentation with 4 spaces, no tabs
- { on the same line preceeded with a space
- space around operators

Especially identation is important because it is such a pain to read JNode code in emacs, less or something like that.

Further thing I personaly like is the use of the Override annotation. IMHO this enhances much the readability because you do not know every interface in JNode. So please let's put "@Override" to the guidlines too!

Javadoc! Yes, please add it to the guidelines! Every public key method, especially public APIs should have javadoc! (This is sadly currently not the case :/)

And what I personaly would like to see, though more input on that is very welcome: Sometimes I see code in JNode which is possibly not SMP safe or even thread safe. Other code should be fixed/changed/whatever but you miss other functionality in JNode to achive that. Eclipse by default treats "// TODO ..." and "// FIXME ..." comments in a special way and you can define more of those keywords. IMHO we should have a predefined set (perhaps TODO and FIXME is already enough) of such comment-keywords to annotate bugs, shortcommings,.. like the above mentioned ones. (BTW, they should be case sensitive to be grepable).

Other options we could discuss about is the use of SuppressWarnings annotation for e.g. nls, but imho that's not so important.

» login or register to post comments

Have you found that document yet Martin?

Submitted by Stephen Crawley on Fri, 04/25/2008 - 04:22.

If you cannot find if (and maybe if you can), I suggest that we express the JNode style rules as "Java Style Guide with the following additions and modifications.". Most experienced Java developers are already familiar with the Java Style Guide.

Using a good (open source) code quality / style checker is a really good idea, especially if the tool allows us to tailor the existing rules and add new ones. (For example, we might want to say that JNode command classes should not print stack traces.)

The other thing we need to do after deciding on style guidelines is to create Eclipse style, etc files that can be checked into SVN. Developers can then import them (by hand) into the relevant JNode projects after doing the initial checkout. (Checking the styles in as project-level settings files is probably a bad idea. That would bring the risk of checking in local / temporary settings by accident.)

» login or register to post comments

Sorry, I can't find the

Submitted by hagar on Mon, 04/28/2008 - 07:39.

Sorry, I can't find the document. I think it was placed at Peter's server and he gave me a link back then. Besides that, I think it might be a good idea to have a chapter in our documentation developer guide that illustrate what ever style we agree upon and also downloadable styles for IntelliJ IDEA and Eclipse would be a good idea.

About the style format, I myself prefer an indentation with 2 spaces. Rasing the '{' to the same line is something I can do again, though I think it gives a better and nice structure of the code to have it on the next line.

» login or register to post comments

Good idea for a task which

Submitted by lsantha on Wed, 04/23/2008 - 14:44.

Good idea for a task which is peding for a long time in the IRC discussions...
Probably this thread could be the place for comming to an agreement on the basic codingstyle guidelines for JNode.
Here are a few points:
- indentation with 4 spaces, no tabs
- { on the same line preceeded with a space
- space around operators
After several refinements we can turn the results into a document and add it to the developers guide.
I would like to see integrated into the build system a source code formatting tool like Jalopy for enforcing the coding style and fixing the old code besides the integrated tools of various IDEs.

» login or register to post comments

PMD

Submitted by galatnm on Wed, 04/23/2008 - 15:32.

A tool like pmd can be helpful to check style during developement and during build. I think that we can define custom rules for PMD too.

My 2 cents,

Fabien L.

» login or register to post comments

good idea :)

Submitted by Fabien D on Wed, 04/23/2008 - 18:10.

Several years ago, I have added pmd to all\build.xml, target checkJNodeRules. But I haven't really used it for a long time and have
left the default rules.

Fabien

my blog : en français, in english or both

» login or register to post comments