patterncsharpMinor
Sequentially displaying a list of game objects
Viewed 0 times
objectsdisplayinggamelistsequentially
Problem
A chat background is created and
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
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).
What is
What is
Inconsistencies
You have bits of code where your
Another inconsistency is your use of
Unused Code
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
This:
Could Be:
Psst...You have a typo:
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:
CreateButton()
Here's a list of the following changes I've made:
-
-
-
-
-
-
(2) This:
```
var nav = new Navigatio
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
varconsistently (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.