-
Notifications
You must be signed in to change notification settings - Fork 5
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
Some code clean up and enhancements #31
Conversation
@PathVariable String digest, @RequestParam(required = false) String level) { | ||
if (level == null) { | ||
level = "none"; | ||
if (!level.equals("1") && !level.equals("2")) { |
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.
Still need to check that level
is non-null here...
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.
Or actually, can you make this the default
case in your switch statement? Then it happens after the null check anyway.
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.
True,
Actually the order of the conditions was not correct.
Just fixed it.
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.
Or actually, can you make this the
default
case in your switch statement? Then it happens after the null check anyway.
case "1"
and case null
have actually the same code, but we can't put case null
in Java (at least for our version), so I think adding the default
case will lead to code duplicate..
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 actually meant something like:
if (level == null) level = "none";
switch (level) {
case "1":
case "none":
...
case "2":
...
default:
return <BAD_REQUEST>
}
But the current ordering is good too (and probably makes more sense) 👍
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.
Ah okay got you,
I think your approach looks more beautiful , so I'll go for it
a89e2b4
to
26ea8b5
Compare
Did some code clean up, loggers, exceptions, info messages, etc