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 bened...@apache.org <bened...@apache.org> 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 <lewandowski.ja...@gmail.com> > *Date: *Monday, 14 March 2022 at 14:27 > *To: *dev@cassandra.apache.org <dev@cassandra.apache.org> > *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 <jmcken...@apache.org> > 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, bened...@apache.org 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) > > > > > > > > > > > > > > >
