Community
Participate
Working Groups
Hi, this is more a "minor" issue while working through the code base of the OTJ compiler and its enhancements to the JDT. The code base seems to have no common style applied. The code is sometimes indented, sometimes it just stands at the first position. This makes reading the code considerable hard. "TeamModel" is one of the better examples, but still it violates many rules of good coding practice. Maybe the project should provide either a style guide or a settings file which includes the style to apply. Easiest would be to use the standard Eclipse style for Java. Then the style should be applied once to the whole code base.
(In reply to Lars Schuetze from comment #0) > Hi, this is more a "minor" issue while working through the code base of the > OTJ compiler and its enhancements to the JDT. > The code base seems to have no common style applied. The code is sometimes > indented, sometimes it just stands at the first position. > > This makes reading the code considerable hard. "TeamModel" is one of the > better examples, but still it violates many rules of good coding practice. > > Maybe the project should provide either a style guide or a settings file > which includes the style to apply. Easiest would be to use the standard > Eclipse style for Java. > Then the style should be applied once to the whole code base. If you talk about code in our fork of org.eclipse.jdt.core then we are limited in what changes we can make without causing big pain during the next merge from upstream. Layout in all classes below org.eclipse.jdt is part of a careful discipline of minimal and traceable changes. Only in org.eclipse.objectteams.* packages we can freely choose the style. Could you give some examples of classes that you find hard to read? I agree that indentation is important for code comprehension, but I never was a big fan of full automatic formatting :). OTOH, if I better understand your difficulties, maybe we can find a solution.
I already thought myself that the layout is like this because of easy upstream merges. What made it hard to read at the beginning is that the code uses so many char[] instead of String. I think this is because of historically reasons. But in newer JVMs (Java 6 onwards) they did a lot to handle string deduplication and related things. There are some occasional inconsistencies like at OTDynCallinBindingsAttribute.BaseMethod. Or nested if's without curly braces that have the same indentation. Or a 450 LOC anonymous subclass in CallinImplementorDyn that is defined as a parameter (that could be refactored. Something I could to because I think I'll touch that code soon).
Created attachment 269781 [details] A screenshot of misaligned source code I think some problems also arise because we use different fonts in the IDE. I use the standard fonts Eclipse uses on MacOS. This could cause misalignment with these probably aligned assignments.
(In reply to Lars Schuetze from comment #2) > I already thought myself that the layout is like this because of easy > upstream merges. > > What made it hard to read at the beginning is that the code uses so many > char[] instead of String. I think this is because of historically reasons. > But in newer JVMs (Java 6 onwards) they did a lot to handle string > deduplication and related things. I see your point. That would, however, be a discussion to be had in JDT, which is not likely to change. In particular any occurence of char[] in public API is here to stay forever. The simplest improvement here: more consistently use helper methods from CharOperation, incl. new ones yet to be written, to avoid at least the notorious System.arraycopy(...) all over the place. > There are some occasional inconsistencies like at > OTDynCallinBindingsAttribute.BaseMethod. what's inconsistent here? > Or nested if's without curly braces that have the same indentation. If there are no curly braces, maybe it is *not* nested and same indentation is correct? I had discussions about braceless constructs with colleagues before. When changing such code, it's essential to know the quick assist to convert statement to block, otherwise you are indeed at risk of creating if (foo) bar(); zork(); and be surprised that zork() is always called ;p > Or a 450 LOC anonymous subclass in CallinImplementorDyn that is defined as a > parameter (that could be refactored. Something I could to because I think > I'll touch that code soon). There's reason here, too: Logically, generateDispatchMethod(..) creates an entire method with signature and body statements. Technically, we split this into: generate signature now, defer generating the body to later. I'd have to dig deeper for the exact motivation, but it had to do with the need to prepend more statements. Now, the current formatting gives the logical view, and if this were changed, many local variables from the outer method would need to be explicitly passed into the inner anonymous thing. Doable, but adding another kind of boilerplate code. Regarding the screenshot: I still don't know what you call misaligned. The only actual difference to how I see it (i.e. caused by fonts): indentation of second arguments within the line comments. Are you opposed to column layout of assignments? I am aware that some of this layout is unusual, but in many cases there's system behind. I'm still interested to learn, how this makes code difficult to read. The mere fact that the code surprises you, doesn't make a strong point for changing it :) CallinImplementorDyn is complex, but I doubt that a different layout or some refactoring will make it easier to understand all at once. Feel free to prove me wrong :)
