HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Sequentially displaying a list of game objects

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
objectsdisplayinggamelistsequentially

Problem

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 ();
//a list of gameobjects containing text and buttons
List text = new List();
//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 ().position = new Vector2(0f,145f);
m.dia.GetComponent ().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

Solution

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

    var t = new List();
    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:

-

  • Added whitespace (Line breaks) for a bit of readability. This doesn't affect performance at all.



-

  • Made use of object initializers.



-

  • Potentially reduce unnecessary calls (see below).



-

  • Used var consistently (aka when not necessary to use explicit form).



-

  • Removed unnecessary spaces before () and [] (see 2nd suggestion in answer).



-

  • 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 Navigatio

Code Snippets

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

m.dia.GetComponent<RectTransform> ().position = new Vector2(0f,145f);
private GameObject cont = null;
cont = button;
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));
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));

Context

StackExchange Code Review Q#132812, answer score: 2

Revisions (0)

No revisions yet.