Best coding Style - Checking if a character is a valid character

Question

I am coding a method that returns whether a given character is valid, which looks like this: -

private static boolean isValid(char c) {
    return c == '.' || c == ',' || c == '+' || c == '/' || c == ';' || c == ':';
}

Check style flagged this up as the boolean complexity is too great (5 when it should be no more than 3). My development manager has flagged up a few alternative implementations which I will post as answers. Personally, I think my code is readable enough and would prefer to turn off check style for this method.

What do you think?

This method makes it more of a pain to add characters or remove them compared to others, and harder to see at a glance which characters are valid. — Brian Ramsay, Aug 12 '09 at 16:23
Make your poll answers c/w or it looks like reputation farming. — Pete Kirkham, Aug 12 '09 at 16:29
@Tom: Using regular expressions for this would work but be very slow (in comparison) and give no added value at all apart from maybe the satisfaction in showing others that your hammer can be used with any nail. — Fredrik, Aug 12 '09 at 17:47
Why has this been closed? It is a real question and I made the answers community wiki. — Tarski, Aug 13 '09 at 08:03

score 12 · Answer 1 · edited Aug 12 '09 at 17:52

12

private static boolean isValid(char c) {
    String validChars =".,+/;:";
    return (validChars.indexOf(c) > -1);
}

edited Aug 12 '09 at 17:52

BenM

4,056
3
24
26

answered Aug 12 '09 at 16:20

Tarski

5,360
4
38
47

This is an ok solution, but it does not scale well. For that, you need a lookup table. – Steven Sudit Aug 12 '09 at 16:25
I'd extract what's a valid character into a property file so you don't have to recompile the code to change the list of valid characters. – Chris Kessel Aug 12 '09 at 16:43

score 5 · Answer 2 · edited Aug 12 '09 at 17:54

5

private static boolean isValid(char c) {
    switch (c) {
    case '.' : // FALLTHROUGH
    case ',' : // FALLTHROUGH
    case '+' : // FALLTHROUGH
    case '/' : // FALLTHROUGH
    case ';' : // FALLTHROUGH
    case ':' :
      return true;
    default : return false;
    }
}

edited Aug 12 '09 at 17:54

BenM

4,056
3
24
26

answered Aug 12 '09 at 16:21

Tarski

5,360
4
38
47

In the case of alternative case statements, I wouldn't bother with the //FALLTHROUGH comments. – Avi Aug 12 '09 at 16:25
I like this one the best because it's the simplest. If speed is not an issue I ALWAYS go for readiblity because you never know who is going to have to maintain your code when you are not around. – Ben Aug 12 '09 at 17:45
I quite dislike this one because it's bulky and inflexible. – Steven Sudit Aug 13 '09 at 13:35

score 2 · Answer 3 · edited Aug 12 '09 at 17:56

2

private static boolean isValid(char c) {
    /* CHECKSTYLE:OFF */
    return c == '.' || c == ',' || c == '+' || c == '/' || c == ';' || c == ':';
    /* CHECKSTYLE:ON */
}

edited Aug 12 '09 at 17:56

BenM

4,056
3
24
26

answered Aug 12 '09 at 16:24

Tarski

5,360
4
38
47

score 2 · Answer 4 · answered Aug 12 '09 at 19:22

I'd use a Set. It has the benefit of having a descriptive name, and it scales well.

private static Set<Character> validCharacters = new HashSet<Character>();

public static void initValidCharacters() {
    validCharacters.add('.');
    validCharacters.add(',');
    validCharacters.add('+');
    validCharacters.add('/');
    validCharacters.add(';');
    validCharacters.add(':');
}

private static boolean isValid(char c) {
    return validCharacters.contains(c);
}

This would work, the only downside would be the auto-boxing overhead on each check? — BenM, Aug 13 '09 at 11:46

score 1 · Answer 5 · edited Aug 12 '09 at 17:55

1

private static boolean isValid(char c) {
    char[] validChars2 = {'.', ',', '+', '/', ';', ':'};
    for (char d : validChars2) {
      if (c == d) { return true; }
    }
    return false;
}

edited Aug 12 '09 at 17:55

BenM

4,056
3
24
26

answered Aug 12 '09 at 16:23

Tarski

5,360
4
38
47

RKitson · Answer 6 · 2009-08-12T18:35:32.717

0

Regular expression could work well. I've hard-coded it into the example below, but you could just as easily pull it out of a config file.

    [Test]
    public void AisNotValid ()
    {
        Assert.IsFalse(IsValid('a'));
    }

    [Test]
    public void SemiColonIsValid ()
    {
        Assert.IsTrue(IsValid(';'));
    }

    public bool IsValid(Char c)
    {
        return Regex.IsMatch(Regex.Escape(".,+/;:"), c.ToString());
    }

The Regex.Escape() method comes in handy here because it escapes characters that normally have meaning in a Regex. From the docs: "Escapes a minimal set of characters (\, *, +, ?, |, {, [, (,), ^, $,., #, and white space) by replacing them with their escape codes."

edited Aug 12 '09 at 18:35

answered Aug 12 '09 at 18:30

RKitson

2,003
1
18
21

Nice, down-voted for using a different (but VERY similar) language? Weak. – RKitson Aug 13 '09 at 19:52
I might downvote on the basis of regexp being overkill, but C# is so similar to Java that I didn't even notice. – Steven Sudit Aug 14 '09 at 13:53

score 0 · Answer 7 · answered Aug 12 '09 at 18:40

0

If readability is not a concern, this can be dealt with a semi binary search, with boolean complexity of 3

As a reference for char values

+ 11
, 12
. 14
/ 15
: 26
; 27

private static boolean isValid(char c)
{
    return c > 14 ? c == '/' || c == ';' || c == ':' : c == '.' || c == ',' || c == '+';
}

answered Aug 12 '09 at 18:40

Bill Yang

1,413
17
28

Binary searches are often faster than linear, but for so few elements, the overhead of the more sophisticated algorithm may well not be worth it. A simple look-up table would be faster, and would remain fast no matter how many characters were being tested for. – Steven Sudit Aug 14 '09 at 13:55

Best coding Style - Checking if a character is a valid character

7 Answers7