Skip to main content

Please review fix for 6466749

2 replies [Last post]
ersh
Offline
Joined: 2006-10-18

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
bkurotsu
Offline
Joined: 2004-12-13

Looks fine. A style comment though...

In the if statement, the "if" is checking to see if we are in verbose mode. You added an else to make sure that i18n is created if we are not (due to decision at top of method). My comment is that the else action is not really related to the verbose setting, logically speaking.

So perhaps you would consider changing it to:

if (verbose)
// print message

if (i18n == null)
i18n = ...

throw ...

I think it makes a bit more logically sense this way, when reading it. You could argue that it requires an additional conditional evaluation than the original, but the performance effect of that is pretty insignificant. :)

Either way you decide, the fix is fine.

ersh
Offline
Joined: 2006-10-18

Yes, you're right, logic in my case was not very clear. Your style is better, I used it. Thanks.