Closed GoogleCodeExporter closed 9 years ago
Hi.
Some comments regarding the changes:
- BruteForceParam.java
- On the lines 46 and 48, consider to set default value of "fileExtensions" to an empty string instead of null (avoids the need to check for null before using it).
- Consider to add a new method that returns a list of file extensions (List<String>), move the validations done in the BruteForce class (lines 111, 112 and 113) to BruteForceParam and change the BruteForce class to call the new method, this way client code (in this case BruteForce) doesn't have to worry about validating/sanitising the file extensions before using them.
- BruteForce.java
- On the line 24, consider to remove the import of the class ArrayList as it's no longer used.
- On the line 111, when checking if the return value of the trim method is empty you should use the method String.equals(String) instead of using the equality operator ("!=") as the later tests if both operands do not refer to the same object (which will be, always, true).
- On the lines 112 and 113, the return value of the split method needs to be sanitised to remove empty strings (in the case the user adds only a comma or two consecutive commas to the file extensions text field).
Best regards.
Original comment by THC...@gmail.com
on 27 Feb 2013 at 3:35
Hi,
Thanks for comments :-) Attached is an updated patch.
>> - BruteForceParam.java
>> - On the lines 46 and 48, consider to set default value of
"fileExtensions" to an
>>empty string instead of null (avoids the need to check for null before using
it).
I'd rather not do this, as I think might be a huge pitfall of java, but from my
experience double-checking an empty string for a null value prevents from
potential application misbehavior when NullPointerException(s) might occur
unpredictably.
In this case I'll have to modify the setter method to ensure that null value
aren't accepted (by throwing IllegalArgumentException or converting null to
""). But in this case there's a choice between this and comparing with null in
getFileExtensionsList() method. I'd rather go with the double-checking for null.
Do you think that this might be wrong from your experience?
Cheers,
Sergey
Original comment by serge....@gmail.com
on 27 Feb 2013 at 7:55
Attachments:
Hi.
If a variable can be null then you'll have to check, always, if it's not null
before using it. The NullPointerException(s) are more likely to happen in this
case, as the (next) developer might forget (or doesn't know) that the variable
can be null at some (unknown) point of execution.
If you ensure that the value is never null there wouldn't be the need for the
extra check (avoiding potential NullPointerException(s)).
Yes, the setter method would need some changes (which is not a bad thing,
throwing the IllegalArgumentException to fail fast).
Why check for null when you don't need to? Reserve that for the cases when the
null value has a "special" meaning or there's no way around it.
Regarding the changes they look good to me (though there's an unused import
(java.util.Arrays) in BruteForceParam.java).
Best regards.
Original comment by THC...@gmail.com
on 2 Mar 2013 at 5:20
Hi,
Thanks for your comments :-) I do consider them appropriate, even though I do
not completely agree with them :-) In this case we'd be able to control what
values are accepted for file extensions list string, so I'd rather go with your
approach.
I'll update the code, test and commit the changes to the trunk (it's been
already rather helpful to me).
Cheers,
Sergey
Original comment by serge....@gmail.com
on 5 Mar 2013 at 11:10
This issue was closed by revision r2944.
Original comment by serge....@gmail.com
on 5 Mar 2013 at 12:47
Original comment by serge....@gmail.com
on 1 Apr 2013 at 8:33
Original comment by psii...@gmail.com
on 2 Apr 2013 at 9:46
Fixed in 2.1.0
Original comment by psii...@gmail.com
on 18 Apr 2013 at 9:49
Original issue reported on code.google.com by
serge....@gmail.com
on 26 Feb 2013 at 12:35Attachments: