bvahdat commented on code in PR #9339:
URL: https://github.com/apache/camel/pull/9339#discussion_r1105496386
##########
components/camel-aws/camel-aws2-kinesis/src/main/java/org/apache/camel/component/aws2/kinesis/consumer/KinesisDefaultResumeAdapter.java:
##########
@@ -46,8 +47,8 @@ public void setRequestBuilder(GetShardIteratorRequest.Builder
resumable) {
@Override
public void resume() {
- assert streamName != null;
- assert resumable != null;
Review Comment:
> These 2 are [invariants which is why checking for assertions is the
correct
approach](https://docs.oracle.com/javase/7/docs/technotes/guides/language/assert.html#usage-invariants).
In normal circumstances they should be defined as final members of the
`KinesisDefaultResumeAdapter` class. Unfortunately our code is made in a way
that does not favor usage of final variables in many places (which then, would
make the assert unnecessary)
In the very same page [it
says](https://docs.oracle.com/javase/7/docs/technotes/guides/language/assert.html#putting-assertions-into-your-code):
> Do not use assertions for argument checking in public methods.
Argument checking is typically part of the published specifications (or
contract) of a method, and these specifications must be obeyed whether
assertions are enabled or disabled. Another problem with using assertions for
argument checking is that erroneous arguments should result in an appropriate
runtime exception (such as IllegalArgumentException, IndexOutOfBoundsException,
or NullPointerException). An assertion failure will not throw an appropriate
exception.
And by Camel we additionally make a differentiation of about if that given
public method is a _user-facing public code_ or not and then we apply the
assert approach or not depending on that? As a concrete example consider the
change by *this* conversation here. Even if this class is not a _user-facing
public code_, according to the recommendation :point_up_2: that assert is done
inside it's contract implementation method, namely
`org.apache.camel.resume.ResumeAdapter#resume` which is wrong according to the
recommendation :point_up_2:.
```
@Override
public void resume() {
assert streamName != null;
assert resumable != null;
...
...
}
```
But I will apply the changes the way you are asking me to do, it's alright 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
