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

[Daniel Fuchs]

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