Skip to main content
added 31 characters in body
Source Link
TorbenPutkonen
  • 9.3k
  • 21
  • 29

Welcome to Code Review!

Bad naming

The method doesn't actually check that a URL is valid, as it allows values that are not URLs. Thus it should not be named as "isValidURL". It should be isValidAddress or similar, but not URL. URLs have a very well defined syntax.

Unnecessary variables

There is no need to store the return value of specialCharactersExists(url) to a variable. It only adds an unnecessary large if-statement. Instead, check the value and exit early. Also, the characters you check are not special, they're "illegal" or "invalid" in your implementation so change name to reflect that:

    if (illegalCharactersExist(url)) {
        return false;
    }

Missing final

Variables that are not supposed to change should be marked as final. If you decide to keep the containsSpecialCharacters variable, it should be marked as final (and named as decribed above).

final boolean containsIllegalCharacters = illegalCharactersExist(url);

Reuse Pattern

The Pattern class is thread safe. You should compile the pattern once into a static variable and reuse it in the matcher. Also, regex is not a good name. It tells what the variable is, not what it's purpose is. Naming should always reflect purpose.

    private static final Pattern ILLEGAL_CHAR_PATTERN = Pattern.compile("[^A-Za-z0-9.-]");

Bad naming

The method doesn't actually check that a URL is valid, as it allows values that are not URLs. Thus it should not be named as "isValidURL". It should be isValidAddress or similar, but not URL. URLs have a very well defined syntax.

Unnecessary variables

There is no need to store the return value of specialCharactersExists(url) to a variable. It only adds an unnecessary large if-statement. Instead, check the value and exit early. Also, the characters you check are not special, they're "illegal" or "invalid" in your implementation so change name to reflect that:

    if (illegalCharactersExist(url)) {
        return false;
    }

Missing final

Variables that are not supposed to change should be marked as final. If you decide to keep the containsSpecialCharacters variable, it should be marked as final (and named as decribed above).

final boolean containsIllegalCharacters = illegalCharactersExist(url);

Reuse Pattern

The Pattern class is thread safe. You should compile the pattern once into a static variable and reuse it in the matcher. Also, regex is not a good name. It tells what the variable is, not what it's purpose is. Naming should always reflect purpose.

    private static final Pattern ILLEGAL_CHAR_PATTERN = Pattern.compile("[^A-Za-z0-9.-]");

Welcome to Code Review!

Bad naming

The method doesn't actually check that a URL is valid, as it allows values that are not URLs. Thus it should not be named as "isValidURL". It should be isValidAddress or similar, but not URL. URLs have a very well defined syntax.

Unnecessary variables

There is no need to store the return value of specialCharactersExists(url) to a variable. It only adds an unnecessary large if-statement. Instead, check the value and exit early. Also, the characters you check are not special, they're "illegal" or "invalid" in your implementation so change name to reflect that:

    if (illegalCharactersExist(url)) {
        return false;
    }

Missing final

Variables that are not supposed to change should be marked as final. If you decide to keep the containsSpecialCharacters variable, it should be marked as final (and named as decribed above).

final boolean containsIllegalCharacters = illegalCharactersExist(url);

Reuse Pattern

The Pattern class is thread safe. You should compile the pattern once into a static variable and reuse it in the matcher. Also, regex is not a good name. It tells what the variable is, not what it's purpose is. Naming should always reflect purpose.

    private static final Pattern ILLEGAL_CHAR_PATTERN = Pattern.compile("[^A-Za-z0-9.-]");
Source Link
TorbenPutkonen
  • 9.3k
  • 21
  • 29

Bad naming

The method doesn't actually check that a URL is valid, as it allows values that are not URLs. Thus it should not be named as "isValidURL". It should be isValidAddress or similar, but not URL. URLs have a very well defined syntax.

Unnecessary variables

There is no need to store the return value of specialCharactersExists(url) to a variable. It only adds an unnecessary large if-statement. Instead, check the value and exit early. Also, the characters you check are not special, they're "illegal" or "invalid" in your implementation so change name to reflect that:

    if (illegalCharactersExist(url)) {
        return false;
    }

Missing final

Variables that are not supposed to change should be marked as final. If you decide to keep the containsSpecialCharacters variable, it should be marked as final (and named as decribed above).

final boolean containsIllegalCharacters = illegalCharactersExist(url);

Reuse Pattern

The Pattern class is thread safe. You should compile the pattern once into a static variable and reuse it in the matcher. Also, regex is not a good name. It tells what the variable is, not what it's purpose is. Naming should always reflect purpose.

    private static final Pattern ILLEGAL_CHAR_PATTERN = Pattern.compile("[^A-Za-z0-9.-]");