0

So my friend and I are programming Blackjack in Java, and we wanted to test our input fields for the correct input(e.g only number input). So we sat at his PC and he wrote this solution:

    public static boolean testeTextFieldInt(JTextField textField,  int geld) {
    if (!textField.getText().isEmpty()) {
        try {
            if(Integer.parseInt(textField.getText())>0 && Integer.parseInt(textField.getText())<geld ) {
            return true;    
            }
        } catch (NumberFormatException e) {
            return false;
        }
    }
    return false;
}

now I disagree with this solution, because your code shouldn't depend on an error, or am I getting this wrong? so i sat down and wrote this:

    public static boolean checkInput(JTextField textField, int spielerGeld, String eingabe) {

    boolean matched = false;

    switch (eingabe) {

    case "num":
        if (!textField.getText().isEmpty() && textField.getText().matches("^[0-9]*$")) {

            int geldinput = Integer.parseInt(textField.getText());

            if (geldinput > 0 && geldinput < spielerGeld) {
                matched = true;
            }
        }
        break;

    case "string":
        if (!textField.getText().isEmpty() && textField.getText().matches("^[a-zA-Z]*$")) {
            matched = true;
        }
        break;

    default:
        break;
    }
    return matched;
}

Keep in mind, we yet dont have any textfields we have to check, but I just implemented it to get a grasp of how you could do multiple checks within one method.

So now my question is, what code is "better"? and what could we/I do better?

Thanks in advance!

EDIT1: So as some already have mentioned, you say my method is not build up after the Single responsibility principle. But if split up into 'checkInputIsnumber' and checkInputIsString' would the first solution(my friend), still be the "better" one?

EDIT2: Better is defined as in, the method should be of low cyclomatic complexity, easy readability and be easy to maintain in the long run.

TheYaINN
  • 114
  • 10
  • 1
    Depends on your definition of "better". Could be closed for "opinioated question". – Fildor Nov 07 '18 at 08:40
  • But anyway: If you want to have one function to check every case, then split it into functions that check one single case and have the "parent" function only chose one of them. Putting multiple responsibilities into one function is considered "not clean" in all dev teams I happened to have had the pleasure to work in. – Fildor Nov 07 '18 at 08:43
  • @Fildor, so if I get what you say correctly, you would reccomend I split the method into two seperate ones like: 'checkInputIsNumber' and 'checkInputIsString'? – TheYaINN Nov 07 '18 at 08:47
  • Yes, "Single Responsibility" => 1. Is Input a Numer? 2. Is Input a String? 3. Do I check for Number or String? All for the reasons in Diadistis answer. – Fildor Nov 07 '18 at 08:50
  • You shouldn't worry (too much) about runtime until it becomes a problem. In 99% of the cases (number made up completely but you get the gist) it is more important to care about maintainability, readability and testability. And regarding that, I'd score your friend's solution higher. – Fildor Nov 07 '18 at 08:58
  • Thank you very much! we're in our second month of programming. I got all my questions but one answered, I still don't get why the "controlflow by exception" is 'allowed' I would consider this to be a very unclean thing, because if you can avoid erros, shouldn't you do that? – TheYaINN Nov 07 '18 at 09:03
  • 1
    Ok. Checking user input: Absolutely a must-do, yes. But you have two "sorts" of error here: 1. The input _is_ a number but "out of range" 2. The input is not a number (i.e. illegal chars). I, personally, would address those in 2 steps. 1 Function ("Validator" for typed _value_) checking for the range: accepts int in the first place. That one is used after checking if the string input has the correct format ("Validator" for _expected format_). – Fildor Nov 07 '18 at 09:11
  • Furthermore, when dealing with ranges, specify inclusivity explicitly. – Diadistis Nov 07 '18 at 09:17
  • On second thought: You may also consider making the TextField accept only a certain range of characters in the first place. See: https://stackoverflow.com/q/1313390/982149 and https://docs.oracle.com/javase/tutorial/uiswing/components/formattedtextfield.html – Fildor Nov 07 '18 at 09:22
  • the SO comment is really neat! Thanks, though it yet exceeds our needs, but it was nice to read and see this example and maybe I will implement something like this in some later version of our Blackjack ;) – TheYaINN Nov 07 '18 at 10:06

2 Answers2

1

The first approach is much better than the second one.

  1. Single responsibility: You should avoid creating methods that do more than one thing.
  2. Open–closed principle: Your 'validation' is not extensible. Try creating a Validator interface and then an implementation per validation type.
  3. Switch statements increase cyclomatic complexity and make testing harder.

Also, don't use textField.getText() everywhere, it's quite possible that it will change between calls. Assign it to a local variable or even better use a String as your argument and not JText. As Fildor pointed out you correctly avoid using exceptions for flow control and it is indeed better to have a single return point. Having said that, for simple cases when you just parse/check and return, it is acceptable.

Diadistis
  • 12,086
  • 1
  • 33
  • 55
0

You should put every check in a single function. After a while your "all in one function" will be unreadable an unmaintainable. Also it easier to change the checks if they are in single functions. Using try/catch for control flow is no good idea. It is expensive at runtime. It is not a good style and most developers won't expect control flow in a catch block.Excpetions are for exceptional situations.

  • The input not being a number and therefore the parsing to fail _is_ an exception. In C# I'd recommend using TryParse, but that is not available in Java. – Fildor Nov 07 '18 at 08:53