unclebob / fitnesse

FitNesse -- The Acceptance Test Wiki
fitnesse.org
Other
2.04k stars 713 forks source link

Coding style guidelines #171

Closed awulder closed 10 years ago

awulder commented 11 years ago

What are the exact coding style guidelines? I know about the 2-space indent but some files have 4-spaces or even tabs. I can't figure out the import order of the classes or when to use the *.

Is it possible the create such guidelines? Maybe it's even possible to create IDE specific configuration files.

benilovj commented 11 years ago

This is a good question.

The bylawsForCommitters file (which, incidentally, I think should be renamed to CONTRIBUTING as per GitHub conventions, hence my PR #169) posits using a 2 space indent, while most (if not all) other Java projects that I've done have used 4 (as do the Sun/Oracle Java coding conventions as well as the defaults in IntelliJ & Eclipse).

IMHO taking the default Java standards from one of the main IDEs (IntelliJ or Eclipse), rather than coming up with one's own is the easiest route.

awulder commented 11 years ago

The Sun/Oracle Java coding conventions sounds good to me. With these kind of guidelines I can focus more on refactoring, bugs and features instead of other stuff related to the coding style used in that specific file.

@amolenaar What do you think?

stanio commented 11 years ago

At least in Eclipse one could easily set up project specific formatting, so it should be really trivial to make this preference and then go on to refactoring, bugs and features.

On the other hand I'm big proponent on adopting the well established Sun's Java coding conventions for writing Java, especially for open source projects, but I don't like massively changing existing code just to meet new formatting style, also. If it is just the indentation amount difference - I think it is not big deal to continue adhering to it.

One may agree on using new indentation amount for new files, but then preserving the indentation in existing files, also. I'm not sure if this approach is any better than just keeping a single style.

awulder commented 11 years ago

Setting up the IDE isn't the problem. What I actually meant was that if the guidelines are set, you can easily add the config files to the project so contributors can you use these files.

Once the guidelines are set you can reformat the current codebase and put it in a single commit. It's just a matter of seconds and there are tests that will ensure that nothing is broken. This means that there is only one single style and that is also what I prefer.

The main reason why I created this is issue is that, to me, it isn't clear what the current guidlines are. I know about the 2-space indent but I can't figure out the import order, when to use curly braces, etc... when I look at the current codebase.

mgaertne commented 11 years ago

Please take a look into the extras/eclipse folder for the FitNesse code formatter for eclipse. We probably should port this one to IntelliJ, but I think you were suggesting making some adaptions first. And we probably want to hint that out in the bylawsForComitters, if it isn't already.

awulder commented 11 years ago

@mgaertne I will take a look and see if I can convert it to a format for Intellij

awulder commented 11 years ago

I've downloaded Eclipse and imported the formatter config file. If I format the Java files with the formatter config file then 755 files are being modified. It seems that the file is outdated or it hasn't been used at all.

Can someone confirm that the file @mgaertne mentioned is still the one we can use?

mgaertne commented 11 years ago

We should use it. That means that we should update it if it does not reflect the code base, or change the code base.

awulder commented 11 years ago

I think the file and the code base needs to be updated. I will take a look at it and I will also create a Intellij config file. Do you want to keep the 2-space indent? The only thing I'm missing is the order of the imports can someone tell me what the order should be?

jediwhale commented 11 years ago

I'm probably guilty for many of the non-standard files - I used 4 space indent for months until I realized that was non-standard! I use IntelliJ so reformatting the code base and providing Eclipse and Intellij configs would be great.

On 2012-12-28 09:27, Arjan wrote:

I think the file and the code base needs to be updated. I will take a look at it and I will also create a Intellij config file.

— Reply to this email directly or view it on GitHub https://github.com/unclebob/fitnesse/issues/171#issuecomment-11735300.

Cheers, Mike Stockdale

/fit/Sharp http://fitsharp.github.com Syterra Software Inc. http://www.syterra.com

woodybrood commented 11 years ago

I'm probably guilty as well.

On Fri, Dec 28, 2012 at 11:40 AM, Mike Stockdale notifications@github.comwrote:

I'm probably guilty for many of the non-standard files - I used 4 space indent for months until I realized that was non-standard! I use IntelliJ so reformatting the code base and providing Eclipse and Intellij configs would be great.

On 2012-12-28 09:27, Arjan wrote:

I think the file and the code base needs to be updated. I will take a look at it and I will also create a Intellij config file.

— Reply to this email directly or view it on GitHub https://github.com/unclebob/fitnesse/issues/171#issuecomment-11735300.

Cheers, Mike Stockdale

/fit/Sharp http://fitsharp.github.com Syterra Software Inc. http://www.syterra.com

— Reply to this email directly or view it on GitHubhttps://github.com/unclebob/fitnesse/issues/171#issuecomment-11736946.

awulder commented 11 years ago

After looking at the formatting options I can't see how the IDE can force the K&R style. Is it really important to maintain the K&R style?

stanio commented 11 years ago

@awulder, I believe referring to the K&R style generally means "opening brace should be on the same line as the block starts", and not that "functions should have the opening brace on a new line".

If I format the Java files with the formatter config file then 755 files are being modified.

Please note an automatic formatter could not be always correct. It might try to join lines of statements previously broken down at different places the automatic formatter would pick, while still adhering to the style guidelines. Also note, the guideliness are just that - guidelines. If in specific places they lead to a formatting which makes the reading more poor - one could choose to overrule the guideline. For example, if a line exceeds the suggested maximum length of 80 with just a few characters - may be forcing wrapping would not improve the readability and style.

awulder commented 11 years ago

@stanio The K&R style is more then just the "opening brace should be on the same line as the block starts". It also means that if a control statement with only a single statement in its scope than the braces can be omitted. That last part is hard to force.

I'am aware that sometimes you want to overrule the guidelines because of readability or some other reason but I also think that if possible you must follow the guidelines so that the codebase stays clean and readable. If every contributor holds on to his own style than the code base becomes unreadable and harder to review.

You have mentioned the line length of 80 characters. I think this can be increased because of all the high resolution displays nowadays.

stanio commented 11 years ago

The only thing I'm missing is the order of the imports can someone tell me what the order should be?

I think it should generally start with the more general libraries and end with the most specific classes, that is - classes from FitNesse itself. Something like:

java. javax. org. net. com.* fit., fitnesse.

Specifying exact grouping in guidelines could be tricky. Even the need of specific order could make a guideline as above erroneous - if one uses import on demand, depending on the specific class there might be a need of different order so the correct classes could be resolved. I think whoever creates a new file could set up the needed order, and whoever changes the files later should not blindly "re-organize imports" with his IDE, but cleanup just unused imports and add new ones as appropriate.

stanio commented 11 years ago

I'am aware that sometimes you want to overrule the guidelines because of readability or some other reason but I also think that if possible you must follow the guidelines so that the codebase stays clean and readable.

The point is you cannot and should not rely on an automatic formatter blindly. I've already given you example where it could fail without the original code going against the guidelines.

You have mentioned the line length of 80 characters. I think this can be increased because of all the high resolution displays nowadays.

I'm against increasing the suggested line length as over longish lines makes the code much harder to read. If you have that many expressions in a single statement - wrap them on multiple lines. Having wider screens doesn't mean having lines that long will be easier to read.

stanio commented 11 years ago

The K&R style is more then just the "opening brace should be on the same line as the block starts".

As far as I see bylawsForCommitters refers to K&R just for braces:

Braces follow K&R style

Further:

It also means that if a control statement with only a single statement in its scope than the braces can be omitted. That last part is hard to force.

Why should one force this? Doesn't it mean it could be used whatever way - with or without braces?

I strongly suggest you follow the standard Java conventions for anything you're unsure, or haven't found specified in this project - or just use your own preference, until it strikes someone else what you're doing appears against the overall established style. Then the given stuff may be additionally specified in order to be enforced for this project.

awulder commented 11 years ago

I'm wondering if it is still useful to create IDE formatter settings after reading the comments?

stanio commented 11 years ago

Yeah, sure - it is a helper. It could make the 2 spaces indent more obvious, at least.

amolenaar commented 10 years ago

Solved?