Implement a pin keypad, simplify code?

Question

I am creating a keypad in my game. The gameobject I have created has 10 buttons: 0-9. The player has to enter a 4 digit code in order to open a door. As soon as the player enters the first digit, it is displayed on the screen of the keypad.

I have all the basic code working, but I am sure I have done this in a very inefficient fashion. Right now, I have attached the following function to one of my keys, the number 1 key in fact:

    public void Key1() {

    if (digit1entered == false) {

        digit1 = 1;
        displaycode.text = digit1.ToString () + digit2.ToString () + digit3.ToString () + digit4.ToString ();
        print ("First digit entered");
        digit1entered = true;
    } else {
        if (digit1entered == true && digit2entered == false) {

            digit2 = 1;
            digit2entered = true;
            displaycode.text = digit1.ToString () + digit2.ToString () + digit3.ToString () + digit4.ToString ();
            print ("2nd digit entered");
        } else {
            if (digit2entered == true && digit3entered == false) {
                digit3 = 1;
                digit3entered = true;
                displaycode.text = digit1.ToString () + digit2.ToString () + digit3.ToString () + digit4.ToString ();
                print ("3rd digit entered");
            } else {
                if (digit3entered == true && digit4entered == false) {

                    digit4 = 1;
                    digit4entered = true;
                    displaycode.text = digit1.ToString () + digit2.ToString () + digit3.ToString () + digit4.ToString ();
                    print ("4th digit entered");

                }
            }

        }
    }
}

In order to make the entire keypad work, I would need to create 9 more functions like the above, each with updated values 2-0. That means I would end up with 10 functions that are all similar, except for the value they enter. That's bad programming, so how do I avoid that? Oh, and if the above is already poor programming to begin with, all tips are welcome ;-)

It's incredibly difficult to do this well. I'll give you some pointers. — Fattie, Jun 30 '16 at 17:59
I think the best would be to have the player type in a combination and check it if the count is of the correct length and check the combination at that point. And use some function like Joe Blow did for the actual button presses. — Gunnar B., Jun 30 '16 at 18:01
Gun - fundamentally you just *feed it in to a state machine*, as ManO explains. That's the "overall" answer here conceptually. A "string" (with FIFO) performs this role perfectly for such a trivial need. — Fattie, Jun 30 '16 at 18:34

score 5 · Accepted Answer · edited Jun 20 '20 at 09:12

It's remarkably difficult to do this well. You picked a real challenge.

Your first problem, you have to centralize the information flow coming in. Let's do that.

Have a function like this

public void UserClickedButtonNumbered(int digitNumber)
    {
    Debug.Log("that button name is " +digitNumber);
    }

say in your script Complex.cs on some object

Have your ten buttons. In the Inspector, drag to connect ...

note that you can actually type in a number - I typed in "3".

so you'd actually set the values "0" to "9" in there on your ten buttons.

{footnote. you wouldn't do this in a "real" project, since it is orthogonal and would cause the design staff a nuisance. i'd magically look up the number based on the keypad position or something. But this is a great start!}

So you actually connect

all the buttons to the same script and function.

Once you have that working, let us know and we'll figure out the next part!

Next you need a function which never lets a string be more than four characters. If you add one on the end, it chops off the first one.

(IMPORTANT: You'd actually do this with an extension, which is the absolute core of Unity programming. But it's too much too explain all at once. When you have time work on this)

Roughly your function is ..

   private string FourOnly(string s)
    {
    while (s.Length > 4 ) s = s.Substring(1);
    return s;
    }

So now you can do this ...

[System.NonSerialized] public string pin = "";

public void UserClickedButtonNumbered(int digitNumber)
    {
    Debug.Log("that button name is " +digitNumber);
    pin = pin + digitNumber.ToString();
    pin = FourOnly(pin);
    Debug.Log("So far, the user entered: " +pin);
    if ( pin == "1313" ) Debug.Log("code unlocked!");
    }

So you're actually done.

The whole thing only takes about four lines of code. Phew.

Finally! You simply want to display the info in your four "LED displays"

Finally, just update the LEDs display.

You really need to do this with UnityEvent which makes it incredibly easy. (Tutorial here.) But it's too much all at once, so just make a very simple function like this ...

public Text led1;  // Text, or whatever your digits are
public Text led2;
public Text led3;
public Text led4;

private void FixLEDs()
 {
 string show = pin + "       "; // just add many spaces on the end
 led1.text = show[0].ToString(); // "show[n]" is a char, not a string
 led2.text = show[1].ToString();
 led3.text = show[2].ToString();
 led4.text = show[3].ToString();
 }

Easy as pie. So simply call that every time the user touches anything,

Here's the final, complete code:

public void UserClickedButtonNumbered(int digitNumber)
    {
    pin = FourOnly( pin + digitNumber );
    FixLEDs();
    if ( pin == "1313" ) Debug.Log("code unlocked!");
    }

You're done. Go drinking.

Thanks Joe! I'll let you know how it goes. Indeed I am trying to use a single function that handles all numbers. I am trying to implement your suggestion(s) above. I am passing the int values for each button to the function, which makes things a bit easier.... — Killbert, Jun 30 '16 at 18:24
Works like a charm! Was using 3d text for the keypad display, but solved that myself. Thanks Joe! I'll put your picture on the wall next to the keypad in my game! — Killbert, Jun 30 '16 at 20:16

ManOVision · Answer 2 · 2016-06-30T18:14:57.033

1

I won't post actual code here, but more of an assignment for you. You should really look into polymorphism and state machines. Here is one of my favorite videos on polymorphism Google IO Talk. In this are some great pointers on how to remove if statements and case statements from code. I don't have a video for a state machine, but basically for your example, you may want one with 7 states: Start, Key1, Key2, Key3, Key4, Error, Success. Now you could have some code that gathers the input and passes that to the state machine, StateMachine.NextState(input). Your state machine can then jump to what ever the next state should be based on the input. You could have each "state" be an object like the video explains and that state would be in charge of doing it's own logic. You should be able to rewrite your problem using no if statements or case statements. Please post your solution. I'd love to see it.

Notice how, if you get this working, easy it would be to change up your states to accept a 6 digit code or 10 digit code, or an alpha numeric code. This is why polymorphism is so great.

edited Jun 30 '16 at 18:14

answered Jun 30 '16 at 18:12

ManOVision

1,853
1
12
14

It's good for intermediate programmers to look in to state machines. However, note this is a Unity question specifically (and an absolute beginner). As an ECS system, Unity *does a huge amount of work for you*. You wouldn't write a multiplication function in *Mathematica*. Indeed, using the inherent value passing UI functions in Unity, it's only 1 line of code - so that's what OP needs to learn here. It's quite annoying that Unity questions are not flagged more clearly. – Fattie Jun 30 '16 at 18:14
Yep, didn't notice the tag. My filter is set to just C#. I'm unaware of what unity gives for free, but regardless, a mess of if statements and case statements can be eliminated with polymorphism. – ManOVision Jun 30 '16 at 18:18
Quite so, quite so. Indeed you'll see my answer is, precisely all that you recommend. (Implemented in an exceptionally economic way :) ) – Fattie Jun 30 '16 at 18:33
Up voted your answer. My answer here is really just to the OP's final remark "all tips are welcome" :) – ManOVision Jun 30 '16 at 18:37
indeed, I voted up this answer as it's the best in the long run – Fattie Jun 30 '16 at 18:43
You guys put a smile on my face. Even despite the fact I am being referred to as an 'absolute beginner'....seriously, thanks guys: this is really helpful! – Killbert Jun 30 '16 at 19:16

NewDeveloper · Answer 3 · 2016-06-30T18:16:14.637

I would clean it up like this

if (digit1Entered == false) {
    digit1 = 1;
    displaycode.text = 
    print("First digit entered");
    digit1entered = true;
}
else if (digit2Entered == false) {
    digit2 = 1;
    digit2entered = true;
    displaycode.text = dispString;
    print("2nd digit entered");
}
else if (digit3Entered == false) {
    digit3 = 1;
    digit3entered = true;
    displaycode.text = dispString;
    print("3rd digit entered");
}
else if (digit4Entered == false) {
    digit4 = 1;
    digit4entered = true;
    displaycode.text = dispString;
    print("4th digit entered");
}

By the nature of your code and from what I can see it shouldn't ever hit multiple if conditions anyway. If you're setting the true/false conditions correctly (which you are) only the appropriate if statement will be evaluated

EDIT: Solution Two

This idea would require a bit of refactoring but it would be a more "expandable" solution

Attach to each of the buttons an OnClick event that calls a method. The method will then call a centralized manager attached to the main game object. When you call that you will pass the value pressed and the associated image (if required)

GameManagerThingy {
    public int ReqEntries {get;set;}
    private string curSelectedString = "";
    private image[] selectedImages;

    public void AddNumber(string numVal, image img) {
        //Insert image and display it on keypad
        //curSelectedString += numVal

        if (ReqEntries == curSelectedString.length-1) 
            //This means enough buttons have been pressed and whatever
            //Evaluation logic you have for entering correct #'s is here
        else
            //The game would continue
    }
}

ButtonClickScript {
     public image buttonImage {get;set;}
     public string buttonValue {get;set;}

     //Tie an onclick event to your game object for when it is clicked
     public void OnClickFunction() {
         GameObject.FindWithTag("manager").GetComponent<SomeScript>().AddNumber(buttonValue, buttonImage);

     }
}

Never use "else if" - ever. Moreover, you really do it totally different from this. — Fattie, Jun 30 '16 at 17:59
What never you use an else if? This could be engineered to be a switch statement instead but I figured that this worked. — NewDeveloper, Jun 30 '16 at 18:00
@JoeBlow care to elaborate on why you so adamantly oppose else-if? — ardila, Jun 30 '16 at 18:03
It's quite remarkable I explained the one-line-of-code solution, and folks are still working on multi-thouand line solutions :-) — Fattie, Jun 30 '16 at 18:17
I hadn't seen your solution, but my second one is very similar to yours. I was attempting to keep the spirit of what OP had put in my answer originally. You still hadn't explained your disdain for else ifs? I am somewhat curious as to why they are so terrible. Thanks! — NewDeveloper, Jun 30 '16 at 18:19

nicolas2008 · Answer 4 · 2016-06-30T18:26:45.153

0

Use current digit pointer instead of boolean flags and store digits in the array.
Function KeyPressed should be called from each key press handler with an appropriate key value as a parameter.

    int currentDigit = 0;
    char[] digits = new char[4];

    public void KeyPressed(char keyDigit)
    {
        if (currentDigit==digits.Length-1)
        {
            return; // all digits entered
        }

        digits[currentDigit] = keyDigit;

        displaycode.text = string.Empty;
        for (int i = 0; i < digits.Length; ++i)
        {
            displaycode.text += digits[i];
        }

        switch(currentDigit)
        {
            case 0:
                print("First digit entered");
                break;
            case 1:
                print("2nd digit entered");
                break;
            // etc
        }

        ++currentDigit;
    }

edited Jun 30 '16 at 18:26

answered Jun 30 '16 at 17:53

nicolas2008

945
9
11

you're on the right track, but it has to be much more modular than this. – Fattie Jun 30 '16 at 18:00
I just show an idea which may help you to acomplish your task. – nicolas2008 Jun 30 '16 at 18:04
I appreciate that - but n unity in particular it's incredibly easy to do this – Fattie Jun 30 '16 at 18:18