[#1764]feature: add advanced urlcheck #1157
Conversation
|
|
||
| @Override | ||
| public void configure(URL url) { | ||
| setMessage(url.message()); | ||
| this.tldMandatory = url.tldMandatory(); |
There was a problem hiding this comment.
Do we really need to store 2 fields? Can't we just store instance of URL?
Like
public void configure(URL url) {
this.url = url;
}
There was a problem hiding this comment.
I do not know if that is a good way. Maybe ask the author of OVal why he not just copies a reference of the annotations in all of his checks.
https://github.com/sebthom/oval/tree/1.32/src/main/java/net/sf/oval/constraint
| } | ||
|
|
||
| @Override | ||
| public boolean isSatisfied(Object validatedObject, Object value, OValContext context, Validator validator) { | ||
| if (value == null || value.toString().length() == 0) { | ||
| return true; | ||
| } | ||
| return urlPattern.matcher(value.toString()).matches(); | ||
| if (!tldMandatory || !excludeLoopback) { |
There was a problem hiding this comment.
I suggest to change order of if/else. It would make code easier to read.
if (tldMandatory && excludeLoopback) {
return urlPattern.matcher(value.toString()).matches();
}
// ... and here come all the other magic...There was a problem hiding this comment.
yes maybe nicer but that is cosmetic and for me i like the most frequent return at the end of the method.
| if (!tldMandatory || !excludeLoopback) { | ||
| //slow for special cases | ||
| String[] localRegexFragments = new String[regexFragments.length]; | ||
| System.arraycopy(regexFragments, 0, localRegexFragments, 0, regexFragments.length); |
There was a problem hiding this comment.
Creating every time a new array seems to be too resource-consuming. And the code is not really simple.
Can't we just hold 4 regexps in 4 constants? I mean
Pattern localUrlPattern_false_false = Pattern.compile(...);
Pattern localUrlPattern_false_true = Pattern.compile(...);
Pattern localUrlPattern_true_false = Pattern.compile(...);
Pattern localUrlPattern_true_true = Pattern.compile(...);There was a problem hiding this comment.
the if part is only the special case, it will not be called every time
see URL annotation.
tldMandatory default true
excludeLoopback default true
And yes you can do x-regexps. but maybe someone will add someday additional special cases and we need more patterns, then i will ask a similar question Do we really need to store x-fields? for all those patterns?
But maybe it is better to use the configure method to initiate only once the correct pattern and just use it in satisfy.
|
@asolntsev is that version better? |
|
@flybyray I added a comment to the gist: May be this could be a shorter solution: private static String[] origRegex = { ... };
private static String[][] regexFragments = {
origRegex,
replace(origRegex, 4, "(?!(?:10)(?:\\.\\d{1,3}){3})"),
replace(origRegex, 13, ""),
replace(origRegex, 13, "")
};P.S. I agree, all my comments are quite minor (cosmetic). |
|
would need something like this. but is this nicer to read? private static String[] origRegex = { ... };
private static IntFunction[] intf = {
x -> (x == 4) ? "(?!(?:10)(?:\\.\\d{1,3}){3})" : null,
x -> (x == 13) ? "" : null
};
private static String[][] regexFragments = {
origRegex,
replace(origRegex.clone(), intf[0]),
replace(origRegex.clone(), intf[1]),
replace(replace(origRegex.clone(), intf[0]), intf[1])};
private static String[] replace(String[] array, IntFunction<? extends String> generator) {
for (int i = 0; i < array.length; i++) {
final String object = generator.apply(i);
if (object != null) array[i] = object;
}
return array;
} |
replacement for #706