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.
