Apps-android-commons: Require Javadocs for all new methods and classes?

[misaochan]

In our current state, I think our code is still somewhat difficult for a new contributor to get started with. A big part of the reason IMO is insufficient Javadocs. Ideally, I feel that every method and class should have Javadocs, at least brief ones. In that way, people who are unfamiliar with the codebase can get a headstart instead of trying to puzzle out from scratch what method does what.

My question to other collaborators: What do you guys think about requiring Javadocs for all NEW methods and classes that are submitted via PR? After all, the best person to write a Javadoc is the person who created the method/class. This won't fix all of the existing code, but would be a good start. After we have started this, then we can slowly start going through the existing code and adding Javadocs to them.

Thoughts? If everyone is on board with this, we can add this requirement to the pull request template and to beginner documentation, and when we review PRs we should ask them to add Javadocs if needed. There may be a bit of an adjustment period at the start (I personally make the mistake of not writing them sometimes!), but I firmly believe this will be beneficial if we stick it out.

[neslihanturan]

I agree it would improve our code quality. But we should want them to be descriptive too. Sometimes Javadoc is just same with method name, and this is meaningless. We should force our contributors (myself included:)) to write detailed explanations too.

[psh]

I think you're exactly right @neslihanturan - Javadoc comments are next to useless in most cases. If the code in question is so complex that it needs explanation then refactor it to make the intent more obvious. If there's still something of note then the code comment should be about why design choices were applied, what the intent was for doing something a certain way. If we do decide to use Javadoc, let's at least dissuade people from applying the @author tag on there; let's strive for collective code ownership.

[misaochan]

@neslihanturan I agree. :)

@psh I somewhat agree with that sentiment in general, but IMO the issue is that "if the code is so complex that it needs explanation..." is a subjective measurement. "So complex" to an experienced developer would be a very different benchmark from "so complex" to a beginner. And arguably, in an application of any significant size it would be impossible for code to not be "complex" in a beginner's eyes. If we want to accommodate a wide range of experience levels in our contributor base, I feel that comments will help more than hinder.

I agree re: not applying the @author tag.

[misaochan]

Also, I think both of your responses highlight the importance of telling contributors what sort of Javadocs we want them to write (if we do), not just "write Javadocs".

[misaochan]

@whym @maskaravivek What do you guys think?

I guess I will take @nicolas-raoul 's emoji as a "yes". 😄

[maskaravivek]

Though I agree with @psh's concern about sometimes the method/class being too complex and needing refactoring, to start with we can at least add Javadocs to briefly explain what it does. Refactoring the old code base with newly adopted design patterns would always be an ongoing exercise.

As our codebase matures and more and more people start contributing we need some way to explain the intent of the code without them waiting for any of us to reply. I agree with having Javadocs for any new code contributed. It would be a good idea to include this guideline in the contributing doc.

[tanvidadu]

I can start working on this issue!
While going through the code-base I will try to update as many javadocs as I can.

[maskaravivek]

Awesome @tanvidadu! That would be really helpful. :)

We all can contribute towards this goal of having richer java docs. :)

[misaochan]

Thanks for the input, everyone. :) As the majority appear to be on board, I have made the relevant changes to https://github.com/commons-app/apps-android-commons/wiki/Code-style#javadocs and https://github.com/commons-app/apps-android-commons/wiki/Project-maintenance#pull-requests-and-merging-workflow . Please feel free to edit if you think any changes/additions are needed.

Collaborators, please check for this in all new PRs that were submitted after 1 May 2018, and please remind each other (including myself! ;)) if we forget.

[domdomegg]

Think this is an excellent move. Seeing as it's been put in place, can we close this issue?

EDIT - going to close, please reopen if necessary