4
\$\begingroup\$

A chat background is created and text[0] is displayed with a button created under it. The created button iterates thru text[] and finds the next one to display setting all others to inactive and the current one to active. text[] can hold a GameObject with just a text block, or up to 4 buttons.

The code works fine and I just know that it needs a lot of improvement.

This is what the code looks like when being used in-game.

using UnityEngine;
using System.Collections;
using UnityEngine.UI;
using System.Collections.Generic;
using System;

public class Dialogue  {
    //the ui of the game
    static GameObject ui = GameObject.FindGameObjectWithTag ("Canvas");
    //the ui's manager
    static UIManager m = ui.GetComponent<UIManager> ();
    //a list of gameobjects containing text and buttons
    List<GameObject> text = new List<GameObject>();
    //the 'press to continue' button
    private GameObject cont = null;
    //the currently displayed text[]
    int currentlyDisplayed = -1;


    //opens the chat display and creates the first text[0] and  the continue buttons
    public void displayChat(int npc){
        var page = (GameObject)Resources.Load("Interfaces/Chat");
        if (m.dia) {
            Resources.UnloadUnusedAssets ();
            return;
        }
        m.dia = (GameObject)m.instantiate (page);
        m.dia.name = page.name;
        m.dia.transform.SetParent(ui.transform);
        m.dia.GetComponent<RectTransform> ().position = new Vector2(0f,145f);
        m.dia.GetComponent<RectTransform> ().sizeDelta = new Vector2 (Screen.width/2,145);
        text = (findText (npc));
        displayText (0);
        var b = CreateButton();

        if (m.c) {
            if (m.c.activeInHierarchy)
                m.c.SetActive (false);
        }
    }
    //displays text[displayed]
    void displayText(int displayed){
        currentlyDisplayed = displayed;
        text[displayed].SetActive(true);
    }
    //iterates threw the text[] to display the next chat dialogue
    void iterateText(){
        for (int i = 0; i < text.Count; i++) {
            if (text [i].activeInHierarchy) {
                text [i].SetActive (false);
                break;
            }
        }
        currentlyDisplayed++;
        if (currentlyDisplayed >= text.Count) {
            m.closeChat ();
            text.Clear ();
            return;
        }
        text [currentlyDisplayed].SetActive (true);
        if (text [currentlyDisplayed].name == "Text") {
            if (cont) {
                if(!cont.activeInHierarchy)
                    cont.SetActive (true);
            }
        }else if (text [currentlyDisplayed].name == "Button") {
            if (cont) {
                if(cont.activeInHierarchy)
                    cont.SetActive (false);
                }
            }
    }
    //finds what text should be displayed
    List<GameObject> findText(int npc){
        List<GameObject> t = new List<GameObject>();
        switch(npc){
        case 1:
            t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "There are times when you get suckered in  ", 16, Color.white,true,false));
            t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "by drugs and alcohol and sex with women-mmkay.", 16,  Color.white,true,false));
            t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "But its when you do these things too much.", 16,  Color.white,true,false));
            t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "That you've become an adict and must get back in touch!", 16,  Color.white,true,false));//
            t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "You can do it its all up to you mmmmmmmkay", 16,  Color.white,true,false));
            t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "Mhmmmm", 16, Color.white,true,false));
            t.Add(addOptions("button1",() => iterateText(),"button2",() => iterateText()));
            t.Add(addOptions("button1",() => iterateText(),"button2",() => iterateText(),"button3",() => iterateText(),"button4",() => iterateText()));
            t.Add(addOptions("button1",() => iterateText(),"button2",() => iterateText(),"button3",() => iterateText()));
            t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "Ha hahahaha hahahahaha", 16,  Color.white,true,false));
            break;
        }
        for(int i = 0; i<t.Count;i++){
            t [i].SetActive (false);
        }
        return t;
    }
    //hex to color
    Color HexToColor(string hex)
    {
        byte r = byte.Parse(hex.Substring(0,2), System.Globalization.NumberStyles.HexNumber);
        byte g = byte.Parse(hex.Substring(2,2), System.Globalization.NumberStyles.HexNumber);
        byte b = byte.Parse(hex.Substring(4,2), System.Globalization.NumberStyles.HexNumber);
        return new Color32(r,g,b, 255);
    }
    //creates a button
    private GameObject CreateButton(){
        GameObject button = new GameObject("button");
        RectTransform trans = button.AddComponent<RectTransform>();
        trans.sizeDelta = new Vector2 ((Screen.width/2)-20, 25);
        button.transform.SetParent (m.dia.transform);
        button.transform.localPosition = new Vector2((button.gameObject.transform.parent.GetComponent<RectTransform>().sizeDelta.x/2),-126);
        var b = button.AddComponent<Button> ();
        var img = button.AddComponent<Image> ();
        var nav = new Navigation ();
        nav.mode = Navigation.Mode.None;
        b.navigation = nav;
        b.targetGraphic = img;
        img.color = HexToColor ("F1D678FF");
        var col = b.colors;
        col.normalColor = Color.white;
        b.colors = col;
        var t = CreateText (b.transform, (Screen.width/2)-20, 25, "Click here to continue...", 14, Color.blue,false,false);
        t.GetComponent<Text> ().alignment = TextAnchor.MiddleCenter;
        b.onClick.AddListener(() => { iterateText(); });
        button.tag = "Continue";
        cont = button;
        return button;
    }
    //adds 2 options
    private GameObject addOptions(string one, Action fOne,string two, Action fTwo){
        var parent = new GameObject ("Button");
        parent.transform.SetParent (m.dia.transform);
        CreateButton (one,() => fOne(),-22).transform.SetParent (parent.transform);
        CreateButton (two,() => fTwo(),-48).transform.SetParent (parent.transform);
        return parent;
    }
    //adds 3 options
    private GameObject addOptions(string one, Action fOne,string two, Action fTwo,string three, Action fThree){
        var parent = new GameObject ();
        parent.transform.SetParent (m.dia.transform);
        CreateButton (one,() => fOne(),-22).transform.SetParent (parent.transform);
        CreateButton (two,() => fTwo(),-48).transform.SetParent (parent.transform);
        CreateButton (three,() => fThree(),-74).transform.SetParent (parent.transform);
        return parent;
    }
    //adds 4 options
    private GameObject addOptions(string one, Action fOne,string two, Action fTwo,string three, Action fThree,string four, Action fFour){
        var parent = new GameObject ();
        parent.transform.SetParent (m.dia.transform);
        CreateButton (one,() => fOne(),-22).transform.SetParent (parent.transform);
        CreateButton (two,() => fTwo(),-48).transform.SetParent (parent.transform);
        CreateButton (three,() => fThree(),-74).transform.SetParent (parent.transform);
        CreateButton (four,() => fFour(),-100).transform.SetParent (parent.transform);
        return parent;
    }
    //creates a button with custom name and function at a selected y
    private GameObject CreateButton(string but1, Action function, int y){
        GameObject button = new GameObject("button");
        RectTransform trans = button.AddComponent<RectTransform>();
        trans.sizeDelta = new Vector2 ((Screen.width/2)-20, 25);
        button.transform.SetParent (m.dia.transform);
        button.transform.localPosition = new Vector2((button.gameObject.transform.parent.GetComponent<RectTransform>().sizeDelta.x/2),y);
        var b = button.AddComponent<Button> ();
        var img = button.AddComponent<Image> ();
        var nav = new Navigation ();
        nav.mode = Navigation.Mode.None;
        b.navigation = nav;
        b.targetGraphic = img;
        img.color = HexToColor ("F1D678FF");
        var col = b.colors;
        col.normalColor = Color.white;
        b.colors = col;
        var t = CreateText (b.transform, (Screen.width/2)-20, 25, but1, 14,  Color.white,true,false);
        t.GetComponent<Text> ().alignment = TextAnchor.MiddleCenter;
        b.onClick.AddListener(() => { function(); });
        return button;
    }
    private GameObject CreateText(Transform canvas_transform, float x, float y, string text_to_print, int font_size, Color text_color, bool shadow, bool outline)
    {
        GameObject UItextGO = new GameObject("Text");
        UItextGO.transform.SetParent(canvas_transform);
        RectTransform trans = UItextGO.AddComponent<RectTransform>();
        trans.anchoredPosition = new Vector2(0, 0);
        //trans.anchorMin = new Vector2(0,0);
        //trans.anchorMax = new Vector2(1,1);
        trans.sizeDelta = new Vector2 (x, y);
        Text text = UItextGO.AddComponent<Text>();
        if (outline) {
            Outline o = UItextGO.AddComponent<Outline> ();
            o.effectColor = Color.black;
        }
        if (shadow) {
            Shadow s = UItextGO.AddComponent<Shadow> ();
            s.effectColor = Color.black;
        }
        text.text = text_to_print;
        text.fontSize = font_size;
        text.color = text_color;
        text.font = Resources.GetBuiltinResource (typeof(Font), "Arial.ttf") as Font;
        return UItextGO;
    }
}

This is how I initially call this file:

var d = new Dialogue ();
d.displayChat (1);
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! Could you confirm whether the code works as intended? I ask because I noticed that dispalyChat() seems to be misspelled. \$\endgroup\$ Jun 23, 2016 at 7:05
  • \$\begingroup\$ Haha thanks for pointing that out, and i edited the original post. But it works fine, just want help improving it. Also thanks for the welcome !! \$\endgroup\$
    – Sniffle6
    Jun 23, 2016 at 7:06

1 Answer 1

2
\$\begingroup\$

Naming

As BKSpurgeon mentioned, your naming is going to make it hard to understand this code in a few months (maybe even weeks).

if (m.c) {
    if (m.c.activeInHierarchy)
        m.c.SetActive (false);
}

What is m.c? What is m.dia?

Inconsistencies

You have bits of code where your () and [] have a space before them when calling a function or indexer. Personally, I find this very confusing, I've never seen this done (Let me know if there's a reason for this, I'm curious). If there is no reason, I would suggest not having a space before them. Examples below.

if (text [i].activeInHierarchy) {
    text [i].SetActive (false);
    break;
}

m.dia.GetComponent<RectTransform> ().position = new Vector2(0f,145f);

Another inconsistency is your use of var. This implicitly determines at compile-time what the type is, so if your code is obvious what it is returning (e.g. var i = 0; or var transform = gameObject.GetComponent<Transform>();) then I would recommend using it.

Unused Code

private GameObject cont = null;
cont = button;

Is this used for anything?

Repetitive Code

You have a few places where you repeat code. Typically programmer's aim for minimal repetition. For more information read up on the DRY principal (stands for "Don't repeat yourself").

This:

t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "There are times when you get suckered in  ", 16, Color.white,true,false));
t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "by drugs and alcohol and sex with women-mmkay.", 16,  Color.white,true,false));
t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "But its when you do these things too much.", 16,  Color.white,true,false));
t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "That you've become an adict and must get back in touch!", 16,  Color.white,true,false));//
t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "You can do it its all up to you mmmmmmmkay", 16,  Color.white,true,false));
t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "Mhmmmm", 16, Color.white,true,false));

Could Be:

var textWidth = Screen.width / 2 - 35; // No need for parenthesis, It calculates all the same (order of operations).
t.Add(CreateText(m.dia.transform, textWidth, 130, "There are times when you get suckered in  ", 16, Color.white,true,false));
t.Add(CreateText(m.dia.transform, textWidth, 130, "by drugs and alcohol and sex with women-mmkay.", 16,  Color.white,true,false));
t.Add(CreateText(m.dia.transform, textWidth, 130, "But its when you do these things too much.", 16,  Color.white,true,false));
t.Add(CreateText(m.dia.transform, textWidth, 130, "That you've become an addict and must get back in touch!", 16,  Color.white,true,false));
t.Add(CreateText(m.dia.transform, textWidth, 130, "You can do it its all up to you mmmmmmmkay", 16,  Color.white,true,false));
t.Add(CreateText(m.dia.transform, textWidth, 130, "Mhmmmm", 16, Color.white,true,false));

Psst...You have a typo: adict should be addict.

Consider making a list/array of the texts you use for dialog so that it's easier to see the text as a whole, then loop through each one adding it as you loop (see below).

Then you could do something like this:

List<GameObject> findText(int npc)
{
    var screenWidth = Screen.width / 2 - 35;
    var textDialogs = new List<string>()
    {
        "There are times when you get suckered in  ",
        "by drugs and alcohol and sex with women-mmkay.",
        // ...
        "Mhmmmm"
    };

    var t = new List<GameObject>();
    switch (npc)
    {
        case 1:
            foreach (var textDialog in textDialogs)
            {
                t.Add(CreateText(m.dia.transform, screenWidth, 130, textDialog, 16, Color.white, true, false));
            }
            t.Add(addOptions("button1",() => iterateText(),"button2",() => iterateText()));
            t.Add(addOptions("button1",() => iterateText(),"button2",() => iterateText(),"button3",() => iterateText(),"button4",() => iterateText()));
            t.Add(addOptions("button1",() => iterateText(),"button2",() => iterateText(),"button3",() => iterateText()));
            t.Add(CreateText (m.dia.transform, (Screen.width/2)-35, 130, "Ha hahahaha hahahahaha", 16,  Color.white,true,false));
            break;
    }

    for(int i = 0; i < t.Count; i++) {
        t[i].SetActive(false);
    }
    return t;
}

CreateButton()

Here's a list of the following changes I've made:

    1. Added whitespace (Line breaks) for a bit of readability. This doesn't affect performance at all.
    1. Made use of object initializers.
    1. Potentially reduce unnecessary calls (see below).
    1. Used var consistently (aka when not necessary to use explicit form).
    1. Removed unnecessary spaces before () and [] (see 2nd suggestion in answer).
    1. Tried to be a bit more explicit on naming. I don't claim to be the best at naming, so take the name changes with a grain of salt.

(2) This:

var nav = new Navigation ();
nav.mode = Navigation.Mode.None;
b.navigation = nav;

(2) Could Be:

b.navigation = new Navigation()
{
    mode = Navigation.Mode.None
};

(3) This:

button.gameObject.transform.parent.GetComponent<RectTransform>().sizeDelta.x/2 

(3) Could Be:

var trans = button.AddComponent<RectTransform>();
...
trans.sizeDelta / 2

(Please correct me if I'm wrong, but those are the same component right?)

Bringing it all together:

private GameObject CreateButton(string but1, Action function, int y) {

    var screenWidth = Screen.width / 2 - 20;
    var buttonObject = new GameObject("buttonObject");

    var trans = buttonObject.AddComponent<RectTransform>();
    trans.sizeDelta = new Vector2(Screen.width, 25);

    // Again, correct me if I'm wrong, but I believe that
    // "buttonObject.gameObject.transform.parent.GetComponent<RectTransform>()" == "trans" right?
    buttonObject.transform.SetParent(m.dia.transform);
    buttonObject.transform.localPosition = new Vector2(trans.sizeDelta.x / 2, y);

    var button = buttonObject.AddComponent<Button>();
    button.navigation = new Navigation()
    {
        mode = Navigation.Mode.None
    };

    var image = buttonObject.AddComponent<Image>();
    button.targetGraphic = image;
    image.color = HexToColor("F1D678FF");

    button.colors.normalColor = Color.white;

    // NOTE: You could add alignment as a parameter but that's soley your decision. That is a lot of parameters.
    // NOTE: Notice the consistent spacing after each comma. It may seem silly but it adds readability.
    var text = CreateText(button.transform, screenWidth, 25, but1, 14, Color.white, true, false);
    text.GetComponent<Text>().alignment = TextAnchor.MiddleCenter;

    button.onClick.AddListener(() => { function(); });

    return buttonObject;
}

Conclusion:

There's a lot here, and some of it may not entirely apply in your case, but with a grain of salt (I sound like a broken record) I hope this will help you.

\$\endgroup\$
7
  • \$\begingroup\$ There are definitely a lot of things I missed or just didn't have time to mention, so if you don't receive anymore answers, I highly recommend applying these suggestions the best you can (and want) and posting another review/question with the updated code. \$\endgroup\$
    – Shelby115
    Jun 23, 2016 at 23:13
  • \$\begingroup\$ Thank you so much for the detailed reply. I will start reviewing/comparing and implementing your suggestions here soon! I really appreciate the time you have spent helping me! \$\endgroup\$
    – Sniffle6
    Jun 24, 2016 at 2:56
  • \$\begingroup\$ Yes! Trans can equal the rectTransform in the way that you have suggested. The only thing is you didn't add the .x on sizeDelta. I should do that tho, you are correct! And i may misunderstand a little. Do you mean i should try to always use var 'var button = buttonObject.AddComponent<Button>();' or should i use 'Button button = buttonObject.AddComponent<Button>();' Also with the spacing after '()' and '[]' was due to my editor auto adding it in haha. Im also a little confused with your CreateText. Would using that allow the game to still work as shown in the gyazo? \$\endgroup\$
    – Sniffle6
    Jun 24, 2016 at 3:07
  • \$\begingroup\$ And im usingthe' var cont' to store the 'press to continue' button in a variable so i can access it easily in my code. Specifically for the iterateText part where it sets cont to active or inactive \$\endgroup\$
    – Sniffle6
    Jun 24, 2016 at 3:08
  • \$\begingroup\$ In regards to the 'var' suggestion, I was stating that if the right-side makes it obvious what the datatype is then use 'var' otherwise don't. So in the case of the button, I would use var since in plain text it's obvious that it'll be a component of type Button. \$\endgroup\$
    – Shelby115
    Jun 24, 2016 at 13:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.