Universalmediaserver: Update and add IDE style guides

[SubJunk]

We have defined our style guide to be the Google one from https://google.github.io/styleguide/javaguide.html

If we are happy with this, we can possibly replace our current config files with theirs:
https://github.com/google/styleguide/blob/gh-pages/eclipse-java-google-style.xml
https://github.com/google/styleguide/blob/gh-pages/intellij-java-google-style.xml

[SubJunk]

@valib @drakulis do you have any opinion about our code style?
My thoughts are that it is good to use a third-party definition because then developers don't waste time discussing/arguing about it, so it is best to just choose one that we like and stick to that, let them make those decisions so we can focus on important things. Just my opinion.

[drakulis]

Sounds good. 👍

[valib]

@SubJunk we have already set formatting for Eclipse made by Nadahar. I have no objection to use third party definition but do we need than reformat all current code or use that definition only for the new code? Reformatting has some disadvantages. There are some parts of the code made to be better readable but not in accordance of the code style. Automatic formatting will change it.

[SubJunk]

@valib I think there are pros and cons for doing a mass formatting but I lean towards agreeing with you that we use the definition only for new code. For one thing it creates a lot of noise in Git blame if we format everything, suddenly it becomes harder to see more details about the last actual change to a line.

And like you said readability is most important and sometimes we do non-standard things for that (like vertical alignment for GUI code, or heavily indented if-statements).

I don't know about the way Eclipse does formatting but other formatting tools/guides I have used let you ignore things. I regularly use ESlint, TSLint, and StyleLint.
I see that for Java there is a similar thing CheckStyle, I wonder if that is a better option since it looks like it is supported by Eclipse, NetBeans and IntelliJ http://checkstyle.sourceforge.net/beginning_development.html

@valib @drakulis What do you guys think about CheckStyle?

[Nadahar]

@SubJunk It doesn't seem like you ever looked at #606 at all: https://github.com/UniversalMediaServer/UniversalMediaServer/blob/fcb4fe4ee00cdbef67971ee2d7e601733e1e589d/CheckStyle.xml#L1-L173

[SubJunk]

It seems that NetBeans' support for CheckStyle isn't what I would like, but the other IDE I use a lot (VS Code) has some extensions for it which I'm trying, will post my findings soon

[SubJunk]

I have it working in VS Code on-the-fly which is nice. I tried their config for the Google style guide (https://raw.githubusercontent.com/checkstyle/checkstyle/master/src/main/resources/google_checks.xml) and it disagrees with us a lot (it even disallows tabs) so that probably isn't a good choice for us, unless we want to dramatically change our code style. I also checked out the Sun config and that agrees with ours more.
The CheckStyle.xml in our project root agrees with us more and if we wanted to go down that route we could refine it more.

So we have some options:

1) Continue to rely on a custom CheckStyle.xml as our source of code style truth, but keep the Eclipse-specific files too, and refine it to match our style more closely.
2) Continue to rely on a custom CheckStyle.xml as our source of code style truth, and remove the Eclipse-specific files - Eclipse users can use a CheckStyle extension like other editors can - and refine the config to match our style.
3) Keep relying on CheckStyle but switch to using a 3rd-party config like Google's or Sun's, and format our code accordingly.
4) Another option?

@valib @drakulis I won't make any decisions until hearing from you both. I am leaning towards option 1 or 2 now, since it seems that our code disagrees with Google's style a lot more than I thought at first. As a non-Eclipse users I like option 2 because it seems over-complicated to have 4 files for this, but maybe you want to keep them @valib ?

[drakulis]

@SubJunk I'm always positive for tools which are increasing code quality and readability. But I think we are mixing up two things in this topic - both important in terms of code quality. Formatter and checkstyle are two different kind of tools.

• Checkstyle is just a code analyzer / reporter and when used in IDE it will just notify you with some fancy marker on the gutter or underline issues in your code. But developer can easily ignore it and commit dirty code. It is rather a tool used on CI / CD servers like Jenkins / Travis to force clean code on developers - otherwise build will fail.
• Formatter will actually make a changes in the code. It will eg. change space-indention to tabs automatically, add some spaces and line breaks here and there even if developer used to use different style. You can invoke it by some shortcut or assign it in save actions - which is a common pattern to keep code clean. It will also not prevent developer from committing dirty code but will automate some changes to keep it clean.

For me as a developer formatter is more important because using one keyboard shortcut I can easily adjust my code to project requirements. Yes, checkstyle is great but until it is not configured on CI/CD server it is rather passive tool I would say.

As for your question I would vote for option 1. And apart of CheckStyle.xml I would leave at least Eclipse.Formatter.xml which, as I wrote in other issue, can be easily imported in all IDEs used by us. This is the one prepared by @Nadahar, right? We may start using it. Well... I think we have to start using it asap since currently we are using none. Then when somebody disagree with particular rule, may create a ticket and provide some arguments why it should be changed.
And I'm agree that new formatter should be used on currently edited file - not on all files in the project. But I mean on FILE - not only on changed code. Otherwise we will end up with different format styles in one file.

I love Eclipse Cleanup which is like a formatter extension and can automatically fix some checkstyle issues. But it seems to not be supported anywhere else then Eclipse. If it is already synchronized with CheckStyle.xml I would also leave it. It will make life easier for Eclipse users.

I would remove Eclipse.CodeTemplates.xml - it doesn't have any valuable content.

@valib As for preventing autoformatting of parts of code: IntelliJ and Eclipse supports special comment-tags which are able to do it:

String testValue = 
"This code will be formatted";
// @formatter:off
       String testValue2 =         "This code will not be formatted in Eclipse and IntelliJ";
       String otherValueABC =      "This code will not be formatted in Eclipse and IntelliJ";
       String otherValueABCDEF =   "This code will not be formatted in Eclipse and IntelliJ";
// @formatter:on
String testValue3 =                "This code will be formatted";

Unfortunately Netbeans doesn't seems to support that :( There is even issue created about it: https://netbeans.org/bugzilla/show_bug.cgi?id=211546 . It has been created on 2012 so it seems nobody cares :( . Even though I think we should use it and NetBeans devs will have to be aware of it and watch out.

P.S.
I don't like tab-indention either ;) but i can leave with it. There seems to be some kind of holy war between fans of tabs and space indention so I think that putting this under discussion will rise war also here. So you have to take hard decision and choose one by yourself.

[SubJunk]

@drakulis thanks for your thoughts. I used to have strong opinions about tabs/spaces and formatting but now I wish I got back all the time I spent caring about that :p I honestly don't care what style is chosen even if it had space-indentation, but it is easier to go with one that most closely matches what we already have, and I don't want to mess up every single line on the project of git blame since that is a really useful tool for me when tracing our bugs.

About the "two different things" point, I am used to one tool doing both things. In ESLint for example you can run eslint --fix and it will automatically fix the things it warned you about. I also use it on Travis in projects and I agree that it belongs on there ideally, if we decide to really care about formatting. That is a decision we need to make. We have a lot of contributors but @valib is the only other official member right now so I want to know his thoughts before deciding.

I had a look at it seems that IDEs do support automatic fixes based on what CheckStyle found, so it seems it does do both ways: https://stackoverflow.com/questions/8409074/how-can-i-easily-fix-checkstyle-errors

If we are happy to use CheckStyle we should work on our config for it, and when we are happy that it is only reporting things we consider to be problems.
Additionally if we decide to be strict about formatting we can make Travis run CheckStyle on PRs