Skip to content
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

Cleanup deprecated constructor use #3649

Merged

Conversation

BradWalker
Copy link
Member

Cleaning some old code to make it more current..

  • removed use of old deprecated constructor use
  • use autobox/unbox for encapsulation
  • change chain-of-if statement to switch w/ string
  • some code cleanup

^Add meaningful description above

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

  • are all your own work, and you have the right to contribute them.
  • are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

Cleaning some old code to make it more current..

- removed use of old deprecated constructor use
- use autobox/unbox for encapsulation
- change chain-of-if statement to switch w/ string
- some code cleanup
@BradWalker BradWalker merged commit 05c385c into apache:master Feb 21, 2022
break;

default:
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the intention is good but i think this makes it more difficult to read.

try to format it like:

               case "[Z":  typeObj = HprofHeap.BOOLEAN;  break; // NOI18N

Its non-standard but would be compact and easier to read in this particular case. The "// NOI18N" hint must be on the same line as the String literal.

Formatting it like this is also really close to switch expressions of later JDKs. Updating to them would only require the "->" and removal of "break".

But most importantly: we have to be careful with null here. What if vmName is null? The switch will throw an exception, the if-else would do fine.

Comment on lines +342 to +343
case "GET":
// NOI18N
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here: // NOI18N must be on the same line behind case "foo"

@mbien
Copy link
Member

mbien commented Feb 21, 2022

@BradWalker please don't merge PRs without giving others a chance to review - even if it seems trivial.

@BradWalker
Copy link
Member Author

BradWalker commented Feb 21, 2022

@mbien , sorry about that.. I got a little bit aggressive with this.. My sincere apologies.. I'll get it fixed..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants