Opened 4 years ago

Closed 3 years ago

#18733 closed task (implemented)

contributor's guide incl. coding guidelines for java projects

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone:
Component: Metrics Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: #18730 Points:
Reviewer: Sponsor:

Description

Combine the guides from Onionoo and metrics-lib, and place the new guide in a central location, which still needs to be identified; this could be a large document in the central place and a small document in CollecTor referencing the main document.

Child Tickets

Change History (25)

comment:1 Changed 4 years ago by iwakeh

Owner: set to iwakeh
Status: newassigned

comment:3 Changed 4 years ago by karsten

Thanks for posting these links. I read/skimmed all three documents. My first impression is that the Oracle documents are rather lengthy and a little cumbersome to read whereas the Google document seems to be more pratcial. The Google document also matches more closely what we're doing, so we'd have to change fewer things to comply with it. I also realized that I really want to avoid writing or maintaining a similar document for our guidelines and instead adapt our coding practices towards what other people do, maybe with just a few exceptions.

More detailed feedback:

  • The Google document says in Section 3.4.2 that "each class [should] order its members in some logical order, which its maintainer could explain if asked." A while ago I read parts of Robert C. Martin's book Clean Code where he describes the Step-down Rule (http://www.itiseezee.com/?p=119). I would like to suggest we use that in Metrics code bases. That would be one of the exceptions/additions that we'd have to make in addition to reference an existing style guide.
  • I noticed that we're breaking lines differently than the Google document suggests. But those rules make sense to me, so I'm happy to adapt.
  • I'd want to keep avoiding // ... style comments, even though the Google documents says in Section 4.8.6.1 that they're okay.
  • We're planning to violate a few of the Javadoc conventions in Section 7. We'll have to talk about those. Maybe we can violate fewer conventions or come up with a good rationale for what we're doing and add that as exception.
  • The Oracle style guide has some suggestions for indentation that I find quite ugly, including using 4 spaces for indents and aligning subsequent lines with an expression in the previous line.
  • The Oracle Javadoc guide is so long that we cannot reasonably expect Metrics Team members to read through that document before contributing.
  • I also briefly looked at Apache Coding Standards and were thrown off by the suggestion to add line breaks before opening brackets, as if Java was the same as C.

comment:4 Changed 4 years ago by iwakeh

Thanks for your comments!

I agree the Google guide is the most pragmatic and concise one.
As it is under CC-By 3.0 License we could just have the current version with the licensing info as reference document in the metrics-team git with an additional very short document that states the differences and extensions we want.

There are also style guides for other languages which we could adopt.
I suppose there is a Tor C-guide, where?

  • The Google document says in Section 3.4.2 that "each class [should] order its members in some logical order, which its maintainer could explain if asked." A while ago I read parts of Robert C. Martin's book Clean Code where he describes the Step-down Rule (http://www.itiseezee.com/?p=119). I would like to suggest we use that in Metrics code bases. That would be one of the exceptions/additions that we'd have to make in addition to reference an existing style guide.

Well, step-down rule is ok with one minor change: I'd always want the lower level function after its first occurrence.

  • I noticed that we're breaking lines differently than the Google document suggests. But those rules make sense to me, so I'm happy to adapt.

Great, b/c I prefer line breaks before the operator symbol. Doing that it is immediatly clear that such a line is a continuation (even when the white space is missing). (I think it actually comes from math typesetting.)

  • I'd want to keep avoiding // ... style comments, even though the Google documents says in Section 4.8.6.1 that they're okay.

I thinks so, too. The // should be avoided and should only be used to explain some inner workings of the code.

  • We're planning to violate a few of the Javadoc conventions in Section 7. We'll have to talk about those. Maybe we can violate fewer conventions or come up with a good rationale for what we're doing and add that as exception.

This should be part of our additional style guide. And, I'd like to set the rule: No comment is better than a wrong or redundant comment. But we could aim at adding more of the usually required tags with time.

  • The Oracle Javadoc guide is so long that we cannot reasonably expect Metrics Team members to read through that document before contributing.

Yes and pretty old.

  • I also briefly looked at Apache Coding Standards and were thrown off by the suggestion to add line breaks before opening brackets, as if Java was the same as C.

That was why I didn't add a link ;-)

comment:5 in reply to:  4 ; Changed 4 years ago by karsten

Replying to iwakeh:

Thanks for your comments!

I agree the Google guide is the most pragmatic and concise one.
As it is under CC-By 3.0 License we could just have the current version with the licensing info as reference document in the metrics-team git with an additional very short document that states the differences and extensions we want.

Works for me, but let's get team buy-in for this by either discussing it at the next team IRC meeting or repeating this suggestion on the metrics-team@ mailing list and asking for objections. Maybe we'll come up with more suggestions like that at this Thursday's meeting that we can send to the list in a single message.

There are also style guides for other languages which we could adopt.

Yes, I noticed. I can't say how practical those are, but I'd say they're definitely candidates to consider when picking a guide for other languages.

I suppose there is a Tor C-guide, where?

This document looks pretty close to what you're looking for:

https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md

  • The Google document says in Section 3.4.2 that "each class [should] order its members in some logical order, which its maintainer could explain if asked." A while ago I read parts of Robert C. Martin's book Clean Code where he describes the Step-down Rule (http://www.itiseezee.com/?p=119). I would like to suggest we use that in Metrics code bases. That would be one of the exceptions/additions that we'd have to make in addition to reference an existing style guide.

Well, step-down rule is ok with one minor change: I'd always want the lower level function after its first occurrence.

I think I agree with that ordering, but let me give an example and hear from you whether that's what you had in mind:

class A {
  int a;
  int b;
  void m() {
    n();
    p();
  }
  void n() {
    o();
  }
  void o() {}
  void p() {}
}

Your preference is to put o() above of p(), because o() is the lower-level function that gets called in n()? Hmm, or would p() go first, because it's a lower-level function called in m()?

And also just to be sure, attributes would go above all methods in this scheme, not above (or even below) methods where they're first used?

(I agree with the rest of what you're saying, so I'm not repeating that here.)

Thank you, this is important work, even if it doesn't produce a single line of code right now. But it will produce better code very soon. :)

comment:6 Changed 4 years ago by iwakeh

(The two sections below were developed and discussed in #18794; and should be part of the guide)

Dependency requirements

Use only libraries from Debian stable as reference for compiling, packaging, and running CollecTor.
That also includes source generation (which is not in use at the time) and javadoc generation.

Any tools for determining quality metrics and quality reports are unrestricted. But, we have some suggested tasks added to our build file which aim at debian stable and only deviate for a good reason.

Packages currently used

On a java-free Debian stable the following packages have to be installed:

ant junit4 libasm4-java libcommons-codec-java libcommons-compress-java libcommons-lang3-java libgoogle-gson-java liblogback-java liboro-java libslf4j-java libxz-java openjdk-7-jdk

comment:7 in reply to:  5 Changed 4 years ago by iwakeh

Replying to karsten:

I think I agree with that ordering, but let me give an example and hear from you whether that's what you had in mind:

class A {
  int a;
  int b;
  void m() {
    n();
    p();
  }
  void n() {
    o();
  }
  void o() {}
  void p() {}
}

Your preference is to put o() above of p(), because o() is the lower-level function that gets called in n()? Hmm, or would p() go first, because it's a lower-level function called in m()?

That is a tricky question :-)
but the solution listed above looks ok.
Imagine, we're reading source code (on a remote server using) less or similar:

When reading m() and then trying to find out what n() does, it'll be good to have o() immediately below n().
Whereas, when we want to find out what p() is about we scroll down and read p()'s declaration.
Even if p() would use o(), too, that's ok, b/c we scrolled by o()'s declaration already, or read it while finding out about n().

comment:8 Changed 4 years ago by karsten

Actually, if p() would use o(), then o() would be the lower-level function and should go after p().

But going back to the example where p() does not use o(), I think we can both live with the suggested ordering. That would be "depth-first", because we're first explaining n() in full detail and then heading over to explain p(). Works for me.

Another thing that I mentioned above: "And also just to be sure, attributes would go above all methods in this scheme, not above (or even below) methods where they're first used?" I wonder if that was a smart suggestion. In a long class there would be quite some distance between delcaration and usage of an attribute. How about we do the opposite of my earlier suggestion and move attributes immediately above methods where they're first used? For members set in constructors that would be at the top of the class, but for attributes used by certain methods only, that would be closer to their usage.

By the way, should I attempt to reorder attributes and methods in CollecTor's relay descs module? I expect that to produce huge diffs, so you shouldn't have any unmerged branches while I'm working on those changes. Is some time this weekend a good time, or is next week better?

comment:9 Changed 3 years ago by iwakeh

I prefer having all attributes at the beginning of a class. When looking at a java file in a simple text editor it's easy to jump to the very beginning, look at the declaration and then jump back to the method declaration. The declaration might be far away from its first use, but easier to locate.

It would be great if you could find the time to reorder the relay desc module this weekend.
Early/mid next week would be fine also. Knowing that you work on that I'll code elsewhere, most likely Configuration and Main.

comment:10 Changed 3 years ago by iwakeh

A common file structure for java projects should be part of the contributor's guide.

Suggestion (adapted from Maven's standard structure):

src/main/java Application/Library sources
src/main/resources Application/Library resources
src/main/webapp Web application sources
src/test/java Test sources
src/test/resources Test resources
generated all test-reports; classes etc. in subfolders, if necessary
generated/classes compiled classes
generated/testclasses compiled test classes
generated/test-results unit test results
generated/instrument classes instrumented by cobertura
generated/javadoc javadoc

The top-level should hold license, readme, and build.xml.

comment:11 Changed 3 years ago by iwakeh

Started a wiki page summarizing the findings here.

comment:12 in reply to:  9 Changed 3 years ago by karsten

Replying to iwakeh:

I prefer having all attributes at the beginning of a class. When looking at a java file in a simple text editor it's easy to jump to the very beginning, look at the declaration and then jump back to the method declaration. The declaration might be far away from its first use, but easier to locate.

Okay, works for me.

It would be great if you could find the time to reorder the relay desc module this weekend.
Early/mid next week would be fine also. Knowing that you work on that I'll code elsewhere, most likely Configuration and Main.

Done and merged to master!

comment:13 in reply to:  10 Changed 3 years ago by karsten

Replying to iwakeh:

A common file structure for java projects should be part of the contributor's guide.

Suggestion (adapted from Maven's standard structure):

Looks good! I just moved all Java sources to src/main/java/ and pushed to master. (I didn't ask for review, because the last two changes were almost trivial, and just in case I messed up things, we can still revert and fix them. Hope this speeds up things a bit.)

comment:14 Changed 3 years ago by iwakeh

Great, thanks for getting this done!

The file structure change is fine. I'm beginning to add tests with the configuration change; so I can add them immediately to src/test/java.

I'll have a separate commit for the other changes, i.e. moving web and resources to the appropriate location.

I'll also add the structure above to the wiki page.

comment:15 Changed 3 years ago by iwakeh

The file structure change continued in my repo

Just noticed that cobertura doesn't work with the change. Please postpone review.

Last edited 3 years ago by iwakeh (previous) (diff)

comment:16 Changed 3 years ago by karsten

Hmm, I just pushed your branch because it looked good and didn't have issues with the cobertura task. (I had to run ant clean and then ant coverage.)

If you think the issue still persists, please send another patch on top of current master.

In the future, please don't edit comments for important changes but only for typos or minor things. I only receive an email for the original comment text and usually don't look for updates on Trac before working on the ticket. Just add a new comment and I'll have the latest state in my inbox. Thanks!

comment:17 in reply to:  16 Changed 3 years ago by iwakeh

Replying to karsten:

Hmm, I just pushed your branch because it looked good and didn't have issues with the cobertura task. (I had to run ant clean and then ant coverage.)

The task runs fine, the problem only shows when you have tests; then the coverage is not calculated correctly.

If you think the issue still persists, please send another patch on top of current master.

I'll post the patch; it is very small.

In the future, please don't edit comments for important changes but only for typos or minor things. I only receive an email for the original comment text and usually don't look for updates on Trac before working on the ticket. Just add a new comment and I'll have the latest state in my inbox. Thanks!

Good to know! I usually read the trac tickets in trac.

comment:18 Changed 3 years ago by iwakeh

the patch branch

currently, cobertura.ser needs to be in the base directory.

comment:19 Changed 3 years ago by iwakeh

Maybe, after applying the patch. Could you already make the following changes to the javadoc task?

It's along the lines of the metrics-lib javadoc:

<javadoc destdir="${docs}"
         footer="(C) 2016 The Tor Project, Inc."
         doctitle="CollecTor Documentation"
         use="true"
         windowtitle="CollecTor Documentation">
  <classpath refid="classpath"/>
  <fileset dir="${sources}/" includes="**/*.java" />
</javadoc>

comment:20 Changed 3 years ago by karsten

Applied your patch and modified the docs target. Thanks!

comment:21 Changed 3 years ago by iwakeh

Status: assignedneeds_review

Please review Metrics Java Coding Style.

As we now have many tickets to get Metrics' projects up-to-date with the coding guidelines (cf. #19611), the guidelines should be reviewed.
When all is fine, this ticket can be closed.

comment:22 Changed 3 years ago by karsten

There, I made some tweaks to the wiki page. Two things I didn't fix yet though:

  1. The links next to section headers in the "Metrics Team Java Coding Style" look broken, and
  2. Section 3.3.3 suggests to treat com.google imports separately, which should be changed to org.torproject.

I didn't touch those, because I don't know how that would break syncing with newer versions of Google's Java Style Guide. Can you make those fixes, or is it safe for me to just make them?

comment:23 Changed 3 years ago by iwakeh

Thanks for reviewing.

  1. The links near the section headers are fine, but the image is not displayed. It is attached to the page, but it is not found.
  2. That's right, should be org.torproject.

Would be great, if you could fix these!

comment:24 Changed 3 years ago by karsten

Mind taking another look?

comment:25 Changed 3 years ago by iwakeh

Component: Metrics/CollecTorMetrics
Keywords: ctip removed
Resolution: implemented
Status: needs_reviewclosed

Looks great now! Thanks for reviewing and polishing!

I added a note to #19611.
Also changed the component to Metrics/Metrics.

Closing as implemented.

Note: See TracTickets for help on using tickets.