I think it is fine to count generated implementations of interfaces as
interfaces, even if they are not defined. If you would like to explicitly
mention this, that is fine. Though, if I’m perfectly honest, I do not find that
mocking improves testing in many cases (instead making it more tightly coupled
and brittle). But that is a separate discussion.
> Having interfaces encourages better unit tests IMHO.
Having unnecessary and unused interfaces encourages messier code, IMHO.
Premature abstraction is bad. Introduce interfaces, methods or indeed any
concept as and when you need them, for testing or otherwise.
> For the instance() / getInstance() methods - I know it is an additional
> effort, but on the other hand it has many advantages because you can replace
> the singleton for testing
Again, do this as necessary. I think for public instances this is a fine
recommendation, but for private uses it should not be prescribed, only used if
there is an explicit benefit.
> And the continuation indent - currently, when I have IntelliJ configured with
> provided formatting setup, I get something like this
Ah, I thought you meant for lambdas. I’m not sure how best to specify a
continuation indent, or in which contexts it applies – only when there is no
other indentation? Conversely, the following works quite nicely. Typically I
try to ensure the start of the line is as succinct as possible to permit clean
indentation follow-up.
method("aaaaaaaaaaaaa",
"bbbbbbbbbbbbb",
"ccccccccccccc"
)
EndpointState removedState = endpointStateMap.stream(endpoint)
.map()…
From: Jacek Lewandowski <[email protected]>
Date: Monday, 14 March 2022 at 16:45
To: [email protected] <[email protected]>
Subject: Re: Updating our Code Contribution/Style Guide
Regarding interfaces, mocks created by Mockito are not really the
implementations. We also cannot predict tests which will be written in the
future. Having interfaces encourages better unit tests IMHO.
An addendum for exception handling guidelines sounds like a good idea.
For the instance() / getInstance() methods - I know it is an additional effort,
but on the other hand it has many advantages because you can replace the
singleton for testing - replace with a newly created instance for a certain
test case
And the continuation indent - currently, when I have IntelliJ configured with
provided formatting setup, I get something like this:
method(
"aaaaaaaaaaaaa",
"bbbbbbbbbbbbb",
"ccccccccccccc"
)
or
EndpointState removedState = endpointStateMap
.remove(endpoint);
I know it is preferred to move to the previous line, but sometimes it makes the
line much too long due to some nested calls or something else.
On Mon, Mar 14, 2022 at 4:02 PM [email protected]<mailto:[email protected]>
<[email protected]<mailto:[email protected]>> wrote:
Hi Jacek,
> Sometimes, although it would be just a single implementation, interface can
> make sense for testing purposes - for mocking in particular
This would surely mean there are two implementations, one of which is in the
test tree? I think this is therefore already covered.
> For exception handling, perhaps we should explicitly mention in the guideline
> that we should always handle Exception or Throwable (which is frequently
> being catched in the code) by methods from Throwables, which would properly
> deal with InterruptedException
I do not think this properly handles InterruptedException –
InterruptedException that are not to be handled directly should now really be
handled by propagating UncheckedInterruptedException, which is very different
to catching all Throwable. In many cases InterruptedException should be handled
explicitly, however.
I do not think catching Exception or Throwable is the correct solution in most
cases either – we should ideally only do so at the top level at which we want
broad unforeseen problems to be handled, or where we need to take specific
actions to handle exception, in which case we should ideally always rethrow the
Throwable unmolested. I can see some benefit from explicitly outlining these
cases, as it is not trivial to handle exceptions cleanly and correctly. We
could perhaps create an exception handling addendum, perhaps in a separate
page, that goes into greater detail?
> I found it useful to access singletons by getInstance() method rather than
> directly
This can be beneficial for public use cases, but for private use cases it is
oftentimes unhelpful to pollute the code. Also note that the document
explicitly proposes avoiding getX, so we would instead have e.g. a method
called instance(). Happy to add a section for this.
>- "...If a line wraps inside a method call, try to group natural parameters
>together on a single line..." while I'm generally ok with that approach,
>putting each argument in a new line, makes it easier for git / review /
>automatic merge
I personally prefer to optimise for readability, and 10+ lines of single short
parameters badly pollutes a page of code IMO. If there is no consensus on this
we can put it to an indicative vote.
>- imports - why mix org.apache.cassandra with other imports (log4j, google,
>etc.)? I know that order is used for a while, but I was always curious why we
>do that?
I would be happy to revisit these, as we have not been consistent about imports
in the codebase. I do not know why they are as they are.
> - define continuation indent - currently it is 0 characters
An opening brace introduces any necessary indentation (from the start of the
line, which is perfect for legibility). I am somewhat inlined to declare that
braces must be used if the lambda cannot fit on the declaring line.
From: Jacek Lewandowski
<[email protected]<mailto:[email protected]>>
Date: Monday, 14 March 2022 at 14:27
To: [email protected]<mailto:[email protected]>
<[email protected]<mailto:[email protected]>>
Subject: Re: Updating our Code Contribution/Style Guide
Hi,
I was looking at the document and have some thoughts:
- Sometimes, although it would be just a single implementation, interface can
make sense for testing purposes - for mocking in particular
- For exception handling, perhaps we should explicitly mention in the guideline
that we should always handle Exception or Throwable (which is frequently being
catched in the code) by methods from Throwables, which would properly deal with
InterruptedException
- I found it useful to access singletons by getInstance() method rather than
directly (public final static field). When we use getInstance() method, we may
go further and make getInstance return the instance from a provider, which
would by default return the value of final field. However in tests, it could
return the value of some mutable static field from a custom provider. This way
we would be able to easily switch the singleton which is impossible without
reloading a class at the moment
- "...If a line wraps inside a method call, try to group natural parameters
together on a single line..." while I'm generally ok with that approach,
putting each argument in a new line, makes it easier for git / review /
automatic merge
- imports - why mix org.apache.cassandra with other imports (log4j, google,
etc.)? I know that order is used for a while, but I was always curious why we
do that?
- format configurations for IDEs - it seems like IntelliJ can import Eclipse
formatter configuration, so maybe one configuration could be enough
- define continuation indent - currently it is 0 characters
- for unit test assertions - prefer AssertJ assertions over standard junit or
hamcrest - maybe forbid them? AssertJ has much better descriptions of failing
assertions
I hope that we can enforce the rules using checkstyle, otherwise this effort
may have little effect. For the transition, perhaps checkstyle could run on
CircleCI just for the modified files?
Thanks,
jacek
- - -- --- ----- -------- -------------
Jacek Lewandowski
On Mon, Mar 14, 2022 at 1:10 PM Josh McKenzie
<[email protected]<mailto:[email protected]>> wrote:
we should add Python code style guides to it
Strongly agree. We're hurting ourselves by treating our python as a 2nd class
citizen.
if we should avoid making method parameters and local variables `final` - this
is inconsistent over the code base, but I'd prefer not having them. If the
method is large enough that we might mistakenly reuse parameters/variables, we
should probably refactor the met
Why not both (i.e. use final where possible and refactor when at length / doing
too much)? The benefits of immutability are generally well recognized as are
the benefits of keeping methods to reasonable lengths and complexity.
On Mon, Mar 14, 2022, at 7:59 AM, Marcus Eriksson wrote:
Looks good
One thing that might be missing is direction on if we should avoid making
method parameters and local variables `final` - this is inconsistent over the
code base, but I'd prefer not having them. If the method is large enough that
we might mistakenly reuse parameters/variables, we should probably refactor the
method.
/Marcus
On Mon, Mar 14, 2022 at 09:41:35AM +0000,
[email protected]<mailto:[email protected]> wrote:
> 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)
>
>
>
