Java: add guidelines for using assertions #37 #38

damithc · 2017-01-27T08:53:32Z

Fixes #37
Fixes #19

Let's specify guidelines for using Java assertions in a new document
that will accumulate more such higher-level Java guidelines in future.

CanIHasReview-bot · 2017-01-27T09:53:07Z

v1

@damithc submitted v1 for review.

[v1 1/1] Add additional coding guidelines for Java

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/oss-generic/process.git refs/pr/38/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

damithc · 2017-01-27T09:59:23Z

codingStandards/AdditionalGuidelines-Java.md

@@ -0,0 +1,98 @@
+# Additional Guidelines - Java
+
+## Argument validation


I couldn't find a good reference for this section, so had to cook up my own guidelines. Do review with a very critical mind.

acjh · 2017-01-27T15:10:47Z

codingStandards/AdditionalGuidelines-Java.md

+   ```
+
+   Furthermore, place the argument validation code immediately above the first line of code it tries to defend, so that
+   the reader can easily deduce the reason for argument validation.


I always thought to validate early, if at all, to avoid unnecessary processing if it's gonna fail anyway.
Surprisingly, despite searching for the past half hour or so, I couldn't find an authoritative guide.

Indeed @acjh , validating arguments upfront seems like the natural thing to do and I used to do it that way too. However, now that the decision to validate depends on a specific circumstances in the code, it seems safer to put the validation near the 'reason for validation' so that if the 'reason for validation' changes, the validation code too can be changed accordingly. For example, if the 'reason for validation' code is extracted to another method, the validation code can go with it. But I don't feel strongly about it; it's just something I cooked up (and yes, I too could not find a good reference to derive from).

wkurniawan07 · 2017-02-02T08:46:03Z

Sorry for the delay. I'll give a review by tomorrow.

wkurniawan07

Just two minor items.

There are a handful of coding style violations though, all revolving missing whitespace:

Between ) and {
Between if and (

wkurniawan07 · 2017-02-03T15:52:23Z

codingStandards/AdditionalGuidelines-Java.md

+   ```
+
+   In the example below, `key` need not be validated against `null` because a `null` value is going to create a 
+   detectable failure (i.e. a `NullPointerException`) anyway.


While this may be accurate,

NullPointerException is a special case; most developers would try their best to avoid this at all costs rather than "letting it happen and catching it somewhere".

It also depends on the method signature; if the method is designed to "return null/false if the argument is null", then this explanation will not apply anymore.

NullPointerException is a special case; most developers would try their best to avoid this at all costs rather than "letting it happen and catching it somewhere".

That's true. I think 'avoid the NPE at all costs' aspect is addressed by the new Optional feature to some extent. Assuming asserts are being used to identify programmer errors while testing (i.e. they are not enabled in production -- java doesn't have a way to enable asserts when run from an executable jar file), our objective is to identify programmer errors and the choice in this example is between an assertion failure and an NPE. Assertion failure cost us an extra line of code and (i.e. assert key != null) and tells us precisely what parameter is null. NPE happens in the same method, but slightly harder to trace back to the offending parameter.

@damithc

our objective is to identify programmer errors and the choice in this example is between an assertion failure and an NPE. Assertion failure cost us an extra line of code and (i.e. assert key != null) and tells us precisely what parameter is null. NPE happens in the same method, but slightly harder to trace back to the offending parameter.

Java 7 and above actually has an Objects::requireNonNull() method which might be useful in this regard.

Java 7 and above actually has an Objects::requireNonNull() method which might be useful in this regard.

Didn't know such a thing existed :-p But it's not a replacement for assert? Probably it's more useful when we want to have argument checking in public APIs where asserts are not recommended.

Another related feature that might be relevant is annotations https://blogs.oracle.com/java-platform-group/entry/java_8_s_new_type Unfortunately, it's not supported in the standard jdk though.

Just realized my comments above are not correct. I mixed up assertions (covered in a different guideline) with argument validation covered in this section.

@damithc what is the standard for this case if I want to throw a NullPointerException with a custom error message (to help debugging)?

@damithc what is the standard for this case if I want to throw a NullPointerException with a custom error message (to help debugging)?

In that case we can use something like Objects#requireNonNull(Object o, String message) ?

wkurniawan07 · 2017-02-03T15:58:35Z

codingStandards/AdditionalGuidelines-Java.md

+
+   ```java
+   void setName(String name){
+       this.saveName(name);


Not directly related to the guideline, but do we have an agreement on usage of this? I always thought that unless absolutely necessary (like this.name = name;), this should always be dropped.

I'll create a separate issue for it. Will remove this. from here.

CanIHasReview-bot · 2017-02-19T03:30:40Z

v2

@damithc submitted v2 for review.

[v2 1/1] Add additional coding guidelines for Java

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/oss-generic/process.git refs/pr/38/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

damithc · 2017-02-19T03:31:27Z

Please ignore v2. I missed some review comments.

CanIHasReview-bot · 2017-02-19T03:35:26Z

v3

@damithc submitted v3 for review.

[v3 1/1] Add additional coding guidelines for Java

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/oss-generic/process.git refs/pr/38/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

damithc · 2017-02-19T03:37:29Z

V2+V3:

Fixed problems mentioned by reviews
Mentioned Objects#requireNonNull()

The use of asserts in the validation of parameters will result in a generic AssertionError whenever an invalid parameter is passed in. This is not desirable as assertions should not be used for parameter validation. It should be enforced by explict checks that throw specific exceptions. Let's modify all methods in the Commons package that uses exception to follow this new rule See: oss-generic/process#38

CanIHasReview-bot · 2017-02-27T15:02:21Z

v4

@damithc submitted v4 for review.

[v4 1/1] Java: add guidelines for using assertions

(📚 Archive) (📈 Interdiff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/oss-generic/process.git refs/pr/38/4/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

damithc · 2017-02-27T15:05:21Z

v4:

Removed guidelines on argument validation as they are not stable yet. Assertion guidelines are more stable and can be merged sooner.

Argument validation is to be discussed in #71

damithc · 2017-03-04T04:46:56Z

Any more comments about this? If not, i'll be merging soon.

acjh · 2017-03-04T05:52:30Z

codingStandards/AdditionalGuidelines-Java.md

+
+Refer the article 
+_[Programming With Assertions](http://docs.oracle.com/javase/7/docs/technotes/guides/language/assert.html)_ 
+(from Oracle) for more details on the three general guidelines below.


"Refer to the article"

Should we be linking .../javase/8/docs/... instead?

Will update as suggested.

@damithc The first point not corrected in v5?

Oops, missed that. Will correct in next iteration.

acjh · 2017-03-04T06:00:48Z

codingStandards/AdditionalGuidelines-Java.md

@@ -0,0 +1,44 @@
+# Additional Guidelines - Java
+
+## Using assertions


Section headings should use Title Case like in the other coding standards (linked here for convenience):

Java

CSS

HTML

Based on this which we are going to adopt soon, headings should be sentence case. Keep this in sentence case and revise coding standards docs later to match the guideline?

wkurniawan07

LGTM

CanIHasReview-bot · 2017-03-12T03:16:23Z

v5

@damithc submitted v5 for review.

[v5 1/1] Java: add guidelines for using assertions

(📚 Archive) (📈 Interdiff between v4 and v5)

Checkout this PR version locally

git fetch https://github.com/oss-generic/process.git refs/pr/38/5/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

damithc · 2017-03-12T03:17:26Z

v5: changed link from java 7 documentation to java 8 documentation

We have a Java coding standard to specify syntax level guidelines. Higher level guidelines such as when to use assertions are not documented. Let's add a document to specify those additional guidelines for Java. Include in that document guidelines for using assertions.

CanIHasReview-bot · 2017-03-13T14:21:07Z

v6

@damithc submitted v6 for review.

[v6 1/1] Java: add guidelines for using assertions

(📚 Archive) (📈 Interdiff between v5 and v6)

Checkout this PR version locally

git fetch https://github.com/oss-generic/process.git refs/pr/38/6/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

damithc · 2017-03-14T15:07:04Z

Going to merge soon. cc: @pyokagan

damithc force-pushed the 37-additional-java-guidelines branch 2 times, most recently from 86176ba to e14f5b1 Compare January 27, 2017 09:52

CanIHasReview-bot added the s.ToReview label Jan 27, 2017

damithc commented Jan 27, 2017

View reviewed changes

acjh reviewed Jan 27, 2017

View reviewed changes

damithc requested a review from wkurniawan07 January 30, 2017 03:09

wkurniawan07 suggested changes Feb 3, 2017

View reviewed changes

damithc force-pushed the 37-additional-java-guidelines branch from e14f5b1 to 8f78d58 Compare February 19, 2017 03:29

CanIHasReview-bot requested a review from wkurniawan07 February 19, 2017 03:30

damithc force-pushed the 37-additional-java-guidelines branch from 8f78d58 to 0d0a027 Compare February 19, 2017 03:34

damithc mentioned this pull request Feb 19, 2017

Reduce unnecessary asserts se-edu/addressbook-level4#239

Closed

MightyCupcakes mentioned this pull request Feb 22, 2017

Reduce unnecessary asserts #239 se-edu/addressbook-level4#300

Closed

damithc force-pushed the 37-additional-java-guidelines branch from 0d0a027 to 91d71d9 Compare February 27, 2017 14:54

damithc changed the title ~~Document additional Java coding guidelines #37~~ Java: add guidelines for using assertions #37 Feb 27, 2017

acjh suggested changes Mar 4, 2017

View reviewed changes

wkurniawan07 approved these changes Mar 8, 2017

View reviewed changes

damithc force-pushed the 37-additional-java-guidelines branch from 91d71d9 to b6fdb40 Compare March 12, 2017 03:04

CanIHasReview-bot requested a review from acjh March 12, 2017 03:16

damithc force-pushed the 37-additional-java-guidelines branch from b6fdb40 to 2eff23e Compare March 13, 2017 14:20

acjh approved these changes Mar 13, 2017

View reviewed changes

damithc added s.ToMerge and removed s.ToReview labels Mar 14, 2017

damithc merged commit 2b68af8 into master Mar 18, 2017

damithc deleted the 37-additional-java-guidelines branch March 18, 2017 03:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: add guidelines for using assertions #37 #38

Java: add guidelines for using assertions #37 #38

damithc commented Jan 27, 2017 •

edited

Loading

CanIHasReview-bot commented Jan 27, 2017

damithc Jan 27, 2017

acjh Jan 27, 2017

damithc Jan 28, 2017

wkurniawan07 commented Feb 2, 2017

wkurniawan07 left a comment

wkurniawan07 Feb 3, 2017

damithc Feb 5, 2017

pyokagan Feb 5, 2017

damithc Feb 5, 2017

damithc Feb 19, 2017

MightyCupcakes Feb 20, 2017

damithc Feb 20, 2017

wkurniawan07 Feb 3, 2017

damithc Feb 5, 2017

CanIHasReview-bot commented Feb 19, 2017

damithc commented Feb 19, 2017

CanIHasReview-bot commented Feb 19, 2017

damithc commented Feb 19, 2017

CanIHasReview-bot commented Feb 27, 2017

damithc commented Feb 27, 2017

damithc commented Mar 4, 2017

acjh Mar 4, 2017

damithc Mar 4, 2017

acjh Mar 12, 2017

damithc Mar 12, 2017

acjh Mar 4, 2017

damithc Mar 4, 2017

wkurniawan07 left a comment

CanIHasReview-bot commented Mar 12, 2017

damithc commented Mar 12, 2017

CanIHasReview-bot commented Mar 13, 2017

damithc commented Mar 14, 2017

		@@ -0,0 +1,98 @@
		# Additional Guidelines - Java

		## Argument validation

		@@ -0,0 +1,44 @@
		# Additional Guidelines - Java

		## Using assertions

Java: add guidelines for using assertions #37 #38

Java: add guidelines for using assertions #37 #38

Conversation

damithc commented Jan 27, 2017 • edited Loading

CanIHasReview-bot commented Jan 27, 2017

v1

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkurniawan07 commented Feb 2, 2017

wkurniawan07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Feb 19, 2017

v2

damithc commented Feb 19, 2017

CanIHasReview-bot commented Feb 19, 2017

v3

damithc commented Feb 19, 2017

CanIHasReview-bot commented Feb 27, 2017

v4

damithc commented Feb 27, 2017

damithc commented Mar 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkurniawan07 left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 12, 2017

v5

damithc commented Mar 12, 2017

CanIHasReview-bot commented Mar 13, 2017

v6

damithc commented Mar 14, 2017

damithc commented Jan 27, 2017 •

edited

Loading