RFR : JDK-8024352 - MBeanOperationInfo accepts any int value as "impact"

[Ujwal Vangapally]

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.
>>>>>
>>>>>
>>>>
>>>
>>
>