Wondering If Instructors Code Is Acceptable and Clean

Unity Engine
1

I’ve been taking a Unity/C# course and there’s one thing that’s starting to bug me a bit (no pun intended).

Under the void update he has ‘States’ in a particular order. But further down, they do not follow the order as written above. Is this a big deal? Shouldn’t they continue the order as they are first written, to be cleaner/more readable? Or is my ocd just flaring up, and I shouldn’t be worried about it.

void Update () {
-        if (Input.GetKeyDown(KeyCode.Space)) {
-            text.text = "You are in a prison cell, and you want to escape. There are " +
-                        "some dirty sheets on the bed, a mirror on the wall, and the door " +
-                        "is locked from the outside.\n\n" +
-                        "Press S to view Sheets, M to view Mirror and L to view Lock" ;
-        }
+        print (myState);
+        if (myState == States.cell)              {state_cell();}
+        else if (myState == States.sheets_0)     {state_sheets_0();}
+        else if (myState == States.sheets_1)     {state_sheets_1();}
+        else if (myState == States.lock_0)       {state_lock_0();}
+        else if (myState == States.lock_1)       {state_lock_1();}
+        else if (myState == States.mirror)       {state_mirror();}
+        else if (myState == States.cell_mirror)  {state_cell_mirror();}
+        else if (myState == States.freedom)      {state_freedom();}
      }
+   
+    void state_cell () {
+        text.text = "You are in a prison cell, and you want to escape. There are " +
+                    "some dirty sheets on the bed, a mirror on the wall, and the door " +
+                    "is locked from the outside.\n\n" +
+                    "Press S to view Sheets, M to view Mirror and L to view Lock" ;
+        if (Input.GetKeyDown(KeyCode.S))          {myState = States.sheets_0;}
+        else if (Input.GetKeyDown(KeyCode.M))     {myState = States.mirror;}
+        else if (Input.GetKeyDown(KeyCode.L))     {myState = States.lock_0;}
+    }
+   
+    void state_mirror() {
+        text.text = "The dirty old mirror on the wall seems loose.\n\n" +
+                    "Press T to Take the mirror, or R to Return to cell" ;
+        if (Input.GetKeyDown(KeyCode.T))          {myState = States.cell_mirror;}
+        else if (Input.GetKeyDown(KeyCode.R))     {myState = States.cell;}
+    }
+   
+    void state_cell_mirror() {
+        text.text = "You are still in your cell, and you STILL want to escape! There are " +
+                    "some dirty sheets on the bed, a mark where the mirror was, " +
+                    "and that pesky door is still there, and firmly locked!\n\n" +
+                    "Press S to view Sheets, or L to view Lock" ;
+        if (Input.GetKeyDown(KeyCode.S))          {myState = States.sheets_1;}
+        else if (Input.GetKeyDown(KeyCode.L))     {myState = States.lock_1;}
+    }
+  
+    void state_sheets_0 () {
+        text.text = "You can't believe you sleep in these things. Surely it's " +
+                    "time somebody changed them. The pleasures of prison life " +
+                    "I guess!\n\n" +
+                    "Press R to Return to roaming your cell" ;
+        if (Input.GetKeyDown(KeyCode.R))         {myState = States.cell;}
+    }
+   
+    void state_sheets_1() {
+        text.text = "Holding a mirror in your hand doesn't make the sheets look " +
+                    "any better.\n\n" +
+                    "Press R to Return to roaming your cell" ;
+        if (Input.GetKeyDown(KeyCode.R))         {myState = States.cell_mirror;}
+    }
+   
+    void state_lock_0() {
+        text.text = "This is one of those button locks. You have no idea what the " +
+                    "combination is. You wish you could somehow see where the dirty " +
+                    "fingerprints were, maybe that would help.\n\n" +
+                    "Press R to Return to roaming your cell" ;
+        if (Input.GetKeyDown(KeyCode.R))         {myState = States.cell;}
+    }
+   
+    void state_lock_1() {
+        text.text = "You carefully put the mirror through the bars, and turn it round " +
+                    "so you can see the lock. You can just make out fingerprints around " +
+                    "the buttons. You press the dirty buttons, and hear a click.\n\n" +
+                    "Press O to Open, or R to Return to your cell" ;
+        if (Input.GetKeyDown(KeyCode.O))          {myState = States.freedom;}
+        else if (Input.GetKeyDown(KeyCode.R))     {myState = States.cell_mirror;}
+    }

Thanks for any and all input/tips.

2

Makes no difference :slight_smile: If you have an instructor, you could probably ask them that. That’s what they’re for, I’d imagine :slight_smile:

Anyways, hope that helps.

1 Like
3

Not really, however switch might be more fitting for this situation than multiple conditional statements that are evaluating enum values. Also game dialogue is rarely hard coded to scripts because it often needs to be localized or just be easily modifiable for non-programmer.

But something like that is better left for slightly more advanced lessons and so this approach is a lot simpler for this chase.

If you’re woried about clean and readable code, you could take look at the C# coding conventions established by Microsoft for C#.

1 Like
4

Thank you so much, Vipsu. That link helps out.

5

I ain’t going to lie…

That’s some god awful looking code.

I mean… forget the whole adhoc nature of the entire thing. I get that, introductory level programming and all. But jeez, the mess that it is.

  1. Use switches, intro level you should be introduced to this simple concept.

  2. EVERY FRAME the code is setting the text to the the ‘States.cell’ message, adhoc, right in the Update call. Then overwriting it with whatever state it is… why have this? What is this for? The text literally gets set, and then immediately set again.

  3. Why is the text getting set EVERY frame? Why doesn’t it get set when the state changes? We’re using what appears to be a very rudimentary state machine… so implement it as a state machine! You set the text ‘OnEnterState’. Basically instead of setting ‘myState’ on some key press, you should have a method called ‘SetState(States state)’, and in that you put the game into the state desired. Leave Update to just handle the inputs.

I’d do something like this… rudimentary, but gets that adhoc, procedural work flow to it:

public class TextAdventurStateMachine : MonoBehaviour
{

    private States _currentState;
 
    void Start()
    {
        this.ChangeState(States.cell);
    }
 
    private void Update()
    {
        switch(_currentState)
        {
            case States.cell:
                {
                    if(Input.GetKeyDown(KeyCode.S)) this.ChangeState(States.sheets_0);
                    if(Input.GetKeyDown(KeyCode.M)) this.ChangeState(States.mirror);
                    if(Input.GetKeyDown(KeyCode.L)) this.ChangeState(States.lock_0);
                }
                break;
            case States.sheets_0:
                {
                    //like cell, but for sheets_0 inputs
                }
                break;
            case States.sheets_1:
                {
                    //like cell, but for sheets_1 inputs
                }
                break;
            case States.lock_0:
                {
                    //like cell, but for lock_0 inputs
                }
                break;
            case States.lock_1:
                {
                    //like cell, but for lock_1 inputs
                }
                break;
            case States.mirror:
                {
                    //like cell, but for mirror inputs
                }
                break;
            case States.cell_mirror:
                {
                    //like cell, but for cell_mirror inputs
                }
                break;
            case States.freedom:
                {
                    //like cell, but for freedom inputs
                }
                break;
        }
    }
 
    private void ChangeState(States state)
    {
        _currentState = state;
        switch(_currentState)
        {
            case States.cell:
                {
                    text.text = "You are in a....";
                }
                break;
            case States.sheets_0:
                {
                    text.text = "The dirty old mirror...";
                }
                break;
            case States.sheets_1:
                {
                    text.text = "Blah Blah....";
                }
                break;
            case States.lock_0:
                {
                    text.text = "Blah Blah....";
                }
                break;
            case States.lock_1:
                {
                    text.text = "Blah Blah....";
                }
                break;
            case States.mirror:
                {
                    text.text = "Blah Blah....";
                }
                break;
            case States.cell_mirror:
                {
                    text.text = "Blah Blah....";
                }
                break;
            case States.freedom:
                {
                    text.text = "Blah Blah....";
                }
                break;
        }
    }
 
}

THEN… with this, we’d then start abstracting the concepts of it.

Maybe move to introducing a nested class to contain the state information. Introducing people to object identity.

public class TextAdventurStateMachine : MonoBehaviour
{
 
    private Dictionary<States, StateData> _states = new Dictionary<States, StateData>();
 
    private StateData _current;
 
    void Start()
    {
        _states[States.cell] = new StateData() {
            Text = "You are in a....",
            Options = new InputOptions[] {
                new InputOptions() {
                    Key = KeyCode.S,
                    StateToChangeTo = States.sheets_0
                },
                new InputOptions() {
                    Key = KeyCode.M,
                    StateToChangeTo = States.mirror
                },
                new InputOptions() {
                    Key = KeyCode.S,
                    StateToChangeTo = States.lock_0
                }
            }
        };
  
        _states[States.sheets_0] = new StateData() {
            Text = "The dirty old mirror...",
            Options = new InputOptions[] {
                new InputOptions() {
                    Key = KeyCode.T,
                    StateToChangeTo = States.cell_mirror
                },
                new InputOptions() {
                    Key = KeyCode.R,
                    StateToChangeTo = States.cell
                }
            }
        };
  
        //todo - set up rest
  
        this.ChangeState(States.cell);
    }
 
    private void Update()
    {
        if(_current == null) return;
  
        foreach(var opt in _current.Options)
        {
            if(Input.GetKeyDown(opt.Key)) this.ChangeState(opt.StateToChangeTo);
        }
    }
 
    private void ChangeState(States state)
    {
        if(_states.TryGetValue(state, out _current))
        {
            text.text = _current.Text;
        }
    }
 
    [System.Serializable]
    public class StateData
    {
        public string Text;
        public InputOptions[] Options;
    }
 
    [System.Serializable]
    public struct InputOptions
    {
        public KeyCode Key;
        public States StateToChangeTo;
    }
 
}

THEN I’d move to probably some constructor methods, error checking, so on.

THEN I’d move forward to showing how to then implement the ‘Start’ adhoc code in the inspector instead via serialization.

THEN I might move onto a custom inspector that pretties up the interface.

So that way each lesson builds on the previous so you can see why we abstract away from the adhoc implementations. While also having sane adhoc implementations that aren’t ridiculous.

3 Likes
6

haha…That’s what I was wondering. This is a course on Udemy, and it’s the first section where we’re actually creating a type of game.

After seeing the instructor’s code, my ocd started flaring up. Your reply seems daunting for a noob as myself, but I’ll compare the two and try to work things out.

Thank you again for the detailed reply.

7

I tried to think about giving the instructor the benefit of the doubt, that he was purposely writing horrid code for the sake that its easier to understand… but honestly the code is really bad and it would only encourage poor coding habits in beginners.

3 Likes
8

Thanks for the reply. That’s what I was thinking at first, and then he would expand on it and clean it up. But why not teach the correct way from the start. IMO, it’s easier to learn than to unlearn.

9

The right way could be a little complex to learn from the start.

But definitely should be a way that isn’t bad, and could be easily expanded on.

Who knows though, I don’t know your instructor, maybe they had something in plan… or maybe they’re just not very good… or maybe I’m really bad at judging… or maybe they just don’t care.

1 Like
10

Aside from setting the text element every frame, the instruction code is ok. It’s plenty readable and understandable. It serves its purpose as an intro.

11

I’d hope that the intro style would be to write code as if you were a new programmer. Then proceed to explain that while yeah, technically speaking, it works, it really could been done better. Then start demonstrating where and how it can be cleaned up, and also explain why it’s better.

1 Like
12

Exactly…

My issue with the instructor code isn’t that it’s rudimentary. It’s that it’s not a good starting point.

You can’t go through it and take one element and say “ok, we’re going to turn this into better code”, it’s “this entire thing has to be tossed out because it’s no bueno, and now brand new code unrelated to the previous”.