Extend database and domain for 'analysis performed' and 'comment' #311
Conversation
There was a problem hiding this comment.
Hi @wow-such-code
your build fails ;) Please address the failure.
…ttps://github.com/qbicsoftware/data-manager-app into feature/dm-819/extend-domain-for-analysis-performed
There was a problem hiding this comment.
Hi @wow-such-code
Thank you for the changes. I'd like to motivate the use of the Optional class for these optional properties.
Looking forward to your changes :)
...ctmanagement/src/main/java/life/qbic/projectmanagement/application/sample/SamplePreview.java
Outdated
Show resolved
Hide resolved
...ctmanagement/src/main/java/life/qbic/projectmanagement/application/sample/SamplePreview.java
Outdated
Show resolved
Hide resolved
...ctmanagement/src/main/java/life/qbic/projectmanagement/application/sample/SamplePreview.java
Show resolved
Hide resolved
...c/main/java/life/qbic/projectmanagement/domain/project/sample/SampleRegistrationRequest.java
Outdated
Show resolved
Hide resolved
…plication/sample/SamplePreview.java Co-authored-by: Tobias Koch <[email protected]>
…plication/sample/SamplePreview.java Co-authored-by: Tobias Koch <[email protected]>
…plication/sample/SamplePreview.java Co-authored-by: Tobias Koch <[email protected]>
…main/project/sample/SampleRegistrationRequest.java Co-authored-by: Tobias Koch <[email protected]>
| public Optional<String> getAnalysisType() { | ||
| return Optional.ofNullable(analysisType); | ||
| } | ||
| public Optional<String> getComment() { | ||
| return Optional.ofNullable(comment); |
There was a problem hiding this comment.
I read up on it. As soon as you have optional fields you should not use a record. The getter will still give back null so if you call sampleRegistrationRequest.analysisType() you would get null.
We should convert this to a class instead.
There was a problem hiding this comment.
I had this problem when I used the original method names "analysisType()" and "comment()". The suggestion to fix this problem was to explicitly add the "get" to the method name. This difference is also displayed by the IDE:
See first reply here for the solution I used: https://stackoverflow.com/questions/62945049/java-records-with-nullable-components
There was a problem hiding this comment.
yes that happens because there would be two methods with the same signature and name but different return type
public String analysisType() and public Optional<String> analysisType() which is not possible in Java. That is why I recommend overwriting the method and making it a class instead of a record or not using optional in this case but simply returning null and make use of optional in the Sample class.
| this.analysisType = analysisType.orElse(""); | ||
| this.comment = comment.orElse(""); |
There was a problem hiding this comment.
I think the sample itself should have null values in these fields as they are optional for the sample.
There was a problem hiding this comment.
How does this fit with using Optional?
There was a problem hiding this comment.
the sample does have nullable fields and optional getters. the sample registration request can have null values. that is fine by me.
In the initial review, I missed the record for the constructor.
|
Kudos, SonarCloud Quality Gate passed!
|
No description provided.