Hi Kedar,
>
> The question/comment was not about it being pleasant to the eyes :)
> The problem with the single line is that of a space or other white space
> characters. It is always better to use the form that takes String[].
Keep in mind that majority of the calls to chmod method are with a
single arg.
eg. chmod("600", pwdFile); <-- MasterPasswordFileManager.java,
Personally, I don't like creating a String array with just one element.
I can provide a overloading method "chmod" that takes a String array.
But do you really think it is necessary in this case?
I want to move on and check in with this bug fix.
We are boggled with the method defined as "protected", but as it turned
out it is called from a different package and then the argument as array.
It's good that we have a detailed code review, but I didn't expect it to
be this long....
Can someone just tell me if my changes are okay? Should I have another
chmod method to handle array arg?
Thanks,
Jane
>
> Going by the same token, I'd say Runtime.exec should not have provided
> the overloaded method that takes a single String. In that regard too,
> ProcessBuilder is a better choice.
>>
>>
>> Also, in a separate e-mail to Byron, I told him that I don't know if
>> I can log a warning message in server.log. This code is in
>> servermgnt. I'll add the code to check if file exists, but I'd need
>> to wrap the message in an exception so it is handled by the caller
>> which eventually is CLI. CLI will display the message in stdout.
>
>
> Yes, there is no way to log a message to server.log from here. This is
> primarily a client side code. But there too, please don't make any
> assumptions about callers. In the future, there could be a caller of this
> code that is not CLI.
>
>>
>> Jane
>>
>>
>>
>>
>>
>> Byron Nevins wrote:
>>
>>> I would wager that there /are/ no subclasses in other packages. If
>>> so, this is a case where adding "protected" made maintenance harder
>>> and more expensive.
>>>
>>> Ref: http://blogs.sun.com/foo/entry/questionable_use_of_protected_in
>>>
>>>
>>> Bill Shannon wrote:
>>>
>>>> Byron Nevins wrote:
>>>>
>>>>> * It's too bad that the chmod method is declared as protected
>>>>> rather than as nothing (package-private). If it was -- you
>>>>> could
>>>>> just change the caller code (3 places in the package) to
>>>>> pass in
>>>>> an array of args instead of doing all of that painful
>>>>> processing. But, since it is protected there could be
>>>>> another caller of the
>>>>> chmod anywhere in the other millions of lines of code, so
>>>>> forget
>>>>> this idea! Moral for interested readers: don't use protected
>>>>> unless it is necessary.
>>>>
>>>>
>>>>
>>>>
>>>> Can't you find all the subclasses and fix them as necessary?
>>>> This isn't a public API, right?
>>>>
>>>> Or, if you're really paranoid, add another chmod method that
>>>> takes the arg array and change the callers to use it.
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: admin-unsubscribe_at_glassfish.dev.java.net
>>>> For additional commands, e-mail: admin-help_at_glassfish.dev.java.net
>>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: admin-unsubscribe_at_glassfish.dev.java.net
>> For additional commands, e-mail: admin-help_at_glassfish.dev.java.net
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: admin-unsubscribe_at_glassfish.dev.java.net
> For additional commands, e-mail: admin-help_at_glassfish.dev.java.net
>