> I'm in favor of codifying the usage of @NotNull and @Nullable stylistically. > +1
I’m in favour of the use of _one_ of @Nullable and @NotNull, preferably the former since we already use it and it’s more reasonable to have a default of non-null variables, parameters and properties. However, I’m not confident in how to craft guidance for these annotations. I don’t think they should be used in every place a variable or property might be null, only in places where it is surprising or otherwise informative to a reader that they might be null. Annotating every property and variable with @NonNull or @Nullable would seriously pollute the screen, and probably harm legibility more than help. At the very least we should mention @Nullable and invite authors to use it where it aids clarity, but if somebody has a good proposal for better guidance I’m all ears. > I think extra clarity and social pressure around "Never catch Exception or > Throwable unless you explicitly rethrow them" sounds valuable We already stipulate that you should always rethrow exceptions, but this is very vague. I will try to tidy this up. On the whole, though, we have a fail-fast approach to processing commands, so we mostly just propagate, with exception handlers existing only for clean-up purposes (except in particular circumstances, usually involving checked exceptions like InterruptedException). So we mostly do catch Throwable (and rethrow), I think, which is what informed the current vague formulation. > When we reference the "Sun Java coding conventions" can we have a canonical > link Yes, this actually is a modification to our existing style guide here: https://cassandra.apache.org/_/development/code_style.html which has such a link, it was just lost in the copy/paste to the Google Doc, but I will restore on commit. > The guidance on brace placement seems to contradict the Java coding > conventions Yep, this is a legacy we all probably wish we didn’t inherit, but we did and it is too late to change it. > The doc doesn't seem to cover a recommendation for braces with single-line > bodies of conditional/loop statements Actually the guide explicitly permits this, and this is for reasons of the brace rule above. Single statement if/for/while loops can rapidly pollute a method with the additional spacing. The legibility benefits of permitting this elision far outweigh any potential protection from semi-colon mistakes. > I like the section on Method clarity, but I would also call out non-trivial > predicate logic as a candidate for encapsulation in its own method I think it’s technically captured under “computing an intermediate result” but you’re right that we could expand this to expressly include complex predicates – particularly those that can be given a descriptive name. > I would recommend that we strengthen the recommendation for using enums for > Boolean properties for any type that is used in method parameters I’m unsure about this. I am not against it per se, but the more enums we have the more clashes of enum identifiers we have, and this can cause confusion particularly with static imports, and in some cases the Boolean property will have a very obvious effect. I prefer to leave some decisions to the author, since we have expressed a strong preference here for the author to consider. But perhaps a blanket policy would do more good than harm. I could endorse it, and am relatively neutral. From: Josh McKenzie <jmcken...@apache.org> Date: Friday, 13 May 2022 at 19:12 To: dev@cassandra.apache.org <dev@cassandra.apache.org> Subject: Re: Updating our Code Contribution/Style Guide Should we consider @NotNull/@Nullable or other annotations besides @Override? I'm in favor of codifying the usage of @NotNull and @Nullable stylistically. +1 In the exception handling section should we discuss using the most applicable exception type for the handler? I.e. don't catch Exception or Throwable? This probably falls under the don't silently swallow or log exceptions paragraph We have been bitten enough times by swallowing exceptions that I think extra clarity and social pressure around "Never catch Exception or Throwable unless you explicitly rethrow them" sounds valuable. +1 to this as well. On Fri, May 13, 2022, at 1:24 PM, Chen-Becker, Derek wrote: I have a couple of questions/comments (in no particular order): * When we reference the "Sun Java coding conventions" can we have a canonical link to that so that people don't have to make an assumption or try and find the version we're talking about? Are we referring to the (now Oracle) version here, or something else? https://www.oracle.com/java/technologies/javase/codeconventions-contents.html * I would recommend that we strengthen the recommendation for using enums for Boolean properties for any type that is used in method parameters. In my experience the improvement in readability at the call site outweighs the (modest, IMHO) cost of introducing a new enum, and the enum also provides a useful "handle" for providing documentation on the semantics of the flag. There are already a lot of Boolean parameters in use in the codebase and I can take a look at what it would take to clean these up * I like the section on Method clarity, but I would also call out non-trivial predicate logic as a candidate for encapsulation in its own method * Should we consider @NotNull/@Nullable or other annotations besides @Override? * In the exception handling section should we discuss using the most applicable exception type for the handler? I.e. don't catch Exception or Throwable? This probably falls under the don't silently swallow or log exceptions paragraph * The guidance on brace placement seems to contradict the Java coding conventions if we place the opening brace on a new line. Is that intentional or am I misreading the statement? Would it be clearer to link to a specific style as defined somewhere (e.g. https://en.wikipedia.org/wiki/Indentation_style#Variant:_Java) * The doc doesn't seem to cover a recommendation for braces with single-line bodies of conditional/loop statements. In my own experience it makes it easier to read if we uniformly used braces everywhere, but it does look like there are quite a few places in the code where we have unbraced ifs Overall the doc is well written and carefully considered, and I appreciate all of the work that went into it! Cheers, Derek From: "bened...@apache.org" <bened...@apache.org> Reply-To: "dev@cassandra.apache.org" <dev@cassandra.apache.org> Date: Friday, May 13, 2022 at 6:41 AM To: "dev@cassandra.apache.org" <dev@cassandra.apache.org> Subject: RE: [EXTERNAL]Updating our Code Contribution/Style Guide CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. It’s been a couple of months since I opened this discussion. I think I have integrated the feedback into the google doc. Are there any elements anyone wants to continue discussing, or things I have not fully addressed? I’ll take an absence of response as lazy consensus to commit the changes to the wiki. From: bened...@apache.org <bened...@apache.org> Date: Monday, 14 March 2022 at 09:41 To: dev@cassandra.apache.org <dev@cassandra.apache.org> Subject: Updating our Code Contribution/Style Guide Our style guide hasn’t been updated in about a decade, and I think it is overdue some improvements that address some shortcomings as well as modern facilities such as streams and lambdas. Most of this was put together for an effort Dinesh started a few years ago, but has languished since, in part because the project has always seemed to have other priorities. I figure there’s never a good time to raise a contended topic, so here is my suggested update to contributor guidelines: https://docs.google.com/document/d/1sjw0crb0clQin2tMgZLt_ob4hYfLJYaU4lRX722htTo Many of these suggestions codify norms already widely employed, sometimes in spite of the style guide, but some likely remain contentious. Some potentially contentious things to draw your attention to: * Deemphasis of getX() nomenclature, in favour of richer set of prefixes and more succinct simple x() to retrieve where clear * Avoid implementing methods, incl. equals(), hashCode() and toString(), unless actually used * Modified new-line rules for multi-line function calls * External dependency rules (require DISCUSS thread before introducing)
