Thanks for the Review Daniel, made changes as suggested.
webrev :
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.03/
Ujwal.
On 11/9/2017 3:37 PM, Daniel Fuchs wrote:
Hi Ujwal,
Give that there are only 4 legal values to check then maybe
you could check all of them in the test (and not simply 2).
best regards,
-- daniel
On 08/11/2017 18:34, Ujwal Vangapally wrote:
Thanks for the suggestions Roger, Mandy.
below is webrev incorporating review comments.
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.02/
CSR: https://bugs.openjdk.java.net/browse/JDK-8190197
Thanks,
Ujwal
On 11/7/2017 10:48 PM, Ujwal Vangapally wrote:
Hi Roger,
kindly see my understanding below.
On 11/7/2017 9:10 PM, Roger Riggs wrote:
Hi Ujwal,
MBeanOperationInfo:163:
Since the values are fixed, you could more concisely just compare
impact >=0 and impact <= UNKNOWN.
As there are only 4 constants, I thought it would be better to
include them directly instead of depending on there values.
If it is a good practice to do it by comparing there values I will
do it that way.
257/263: I don't see a reason to change the toString in the
default case for getImpact().
As impact can't have any other value than 4 constants, we don't need
the default case any more.
A suggestion would be to introduce an Enum for the action values
and the corresponding new
method; perhaps deprecating the current method (or not).
The enum would use the same values as currently and so internally
the implementation does not
change significantly.
Kindly clarify.
As it changes the constructor signature if we introduce a Enum,
but as we can solve the issue by throwing IAE, do we still need to
introduce Enum and change method signature.
Thanks,
Ujwal.
$.02, Roger
On 11/7/2017 6:05 AM, Ujwal Vangapally wrote:
Kindly review the fix for bug below.
https://bugs.openjdk.java.net/browse/JDK-8024352
webrev :
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.00/
Thanks,
Ujwal.
