-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CWMVUE-611 - Adding in endpoint to get time range map for measurments #975
Conversation
6876dd0
to
5f57059
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall structure looks good. There a few "readability" areas that I think should be addressed.
{ | ||
Timestamp retval = null; | ||
if(date != null) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The curlies on this are wrong.
Also this would probably read easier just doing a return date !=null ? Timestamp.from(date) : null;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to fix brackets and used a ternary
@Schema(description = "Last update in the timeseries") | ||
@JsonFormat(shape = Shape.STRING) | ||
ZonedDateTime lastUpdate; | ||
|
||
@SuppressWarnings("unused") // required so JAXB can initialize and marshal | ||
private TimeSeriesExtents() { | ||
super(new Builder()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just... odd.
The constructor should just take a builder and thus both constructors should be private. We appear to be mixing builder/non-builder patterns here and a rather odd way..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I tried to preserve the existing TimeSeriesExtents object as much as possible, while making the new TimeExtents object follow the builder pattern of our other dtos. I would be in favor of updating the TimeSeriesExtents to use a builder pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the TimeSeriesExtents to use a builder pattern for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks a lot better.
…. Updated measurement retrieval to use XML api. Fixed issues with quality. Introduced TimeExtents object.
…apis in extending TimeExtents. Code refactor.
Co-authored-by: Mike Neilson <[email protected]>
523536a
to
9f7984a
Compare
Updated measurement retrieval to use XML api. Fixed issues with quality. Introduced TimeExtents object.