If, else, if, else, if, else, if, else.... Wait, what?

Code (csharp):

1.  
2. if (details == item1) {
3.     Debug.Log("1 is selected");
4.    }
5.    else {
6.     if (details == item2) {
7.      Debug.Log("2 is selected");
8.     }
9.     else {
10.      if (details == item3) {
11.       Debug.Log("3 is selected");
12.      }
13.      else {
14.       if (details == item4) {
15.        Debug.Log("4 is selected");
16.       }
17.        else {
18.         if (details == item5) {
19.         Debug.Log("5 is selected");
20.         }
21.        }
22.       }
23.      }
24.     }

Annnd, it works fine up until the 4th item. Then it starts printing out 2, and 3, and then if I try to print out 5 it goes crazy and prints out a hundred iterations of all the other print statements. I tried doing this with an if statement, 3 else if statements, and an else statement with the same result. I am very confused and my brain is fried, please some logic god tell me what I did wrong with this logic, this has frustrated me to no end.

 

You could try using an if else statement. Or better yet a switch statement

Edit: Changes from select to switch. To much VB

 

I'm pretty sure I tried that, and such a logic statement does not exist. You are simply expected to use an if and an else statement. And I'm pretty sure else if is just a simplification of what I did. And I have no idea what a select statement is. Maybe you are talking about C#? However, I want to keep this java, since I have like 400 lines of code haha, and converting it would take.... well, a long time.

 

Sorry, its a switch you are looking for. My bad.

 

First fix the indentation, it makes it much easier to read.

 

I'd also recommend a switch statement.

Also, cleaning up the braces, your code looks something like this

Code (csharp):

1.  
2. if (details == item1)
3. {
4.     Debug.Log("1 is selected");
5. }
6. else
7. {
8.     if (details == item2)
9.     {
10.        Debug.Log("2 is selected");
11.     }
12.     else
13.     {
14.         if (details == item3)
15.         {
16.             Debug.Log("3 is selected");
17.         }
18.         else
19.         {
20.             if (details == item4)
21.             {
22.                 Debug.Log("4 is selected");
23.             }
24.             else
25.             {
26.                 if (details == item5)
27.                 {
28.                     Debug.Log("5 is selected");
29.                 }
30.             }
31.         }
32.     }
33. }
34.  

Any reason you aren't writing it like this?:

Code (csharp):

1.  
2. if (details == item1)
3. {
4.     Debug.Log("1 is selected");
5. }
6. else if (details == item2)
7. {
8.     Debug.Log("2 is selected");
9. }
10. else if (details == item3)
11. {
12.     Debug.Log("3 is selected");
13. }
14. else if (details == item4)
15. {
16.     Debug.Log("4 is selected");
17. }
18. else if (details == item5)
19. {
20.     Debug.Log("5 is selected");
21. }
22. else
23. {
24.     Debug.Log("Unknown selected");
25. }
26.  

Again, this pattern easily lends to using a switch block instead.

If this doesn't fix your problem, there is probably an issue with where you are receiving the input, or not reseting the details variable when running this test.

 

Note, a switch statement won't work here... switches can only use constants for each case:

https://msdn.microsoft.com/en-us/library/06tc147t.aspx

 

While I can't say for certain, as other parts of my script are preventing me from testing this, but if you locally define a variable, you can use that in your switch.
For example, my script is looking to see what key is being pressed, so

Code (csharp):

1.  
2. var input = Input.inputString;
3.         switch (input)
4.         {
5.             case "1":
6.                 //do stuff
7.                 break;
8.             case "2":
9.                 //do other stuff
10.                 break;
11.             default:
12.                 break;
13.  

But, again, this is not tested in-engine.

 

even my head going crazy after read ur script
as well as the engine

 

What object type is details(and itemX for that matter)?

EDIT: The point I will be getting to, is that instead of doing a huge amount of if-statements, you should have your objects implement an interface, and delegate the logic to the classes themselves. This ensures encapsulation and extensibility.

EDIT 2: Something like this:

Code (CSharp):

1. interface IItem {
2.     public void debugSelectedPrint();
3. }
4.  
5. class Item : IItem {
6.     String name;
7.  
8.     public Item(String name) {
9.         this.name = name;
10.     }
11.  
12.     public void debugSelectedPrint() {
13.         Debug.Log(name + " is selected");
14.     }
15. }
16.  
17. class Main {
18.     IItem selectedItem;
19.  
20.     debugSelectedPrint() {
21.         selectedItem.debugSelectedPrint();
22.     }
23. }

 

Ah, @lordofduct has a point with the switch statement not being usable in this situation.
The ideal solution would be of course what @Boz0r recommends.

An alternative is to push the values in a List<> or array, loop through them, and test each value.

Some pseudocode would look like:

Code (csharp):

1.  
2. ItemType[] items = { item1, item2, item3, item4, etc }
3.  
4. foreach (ItemType item in items)
5. {
6.    if (details == item)
7.    {
8.         // Do something
9.         break;
10.    }
11. }
12.  

And if you use @Boz0r's solution, you can do something like

Code (csharp):

1.  
2. ItemType[] items = { item1, item2, item3, item4, etc }
3.  
4. foreach (ItemType item in items)
5. {
6.    if (details == item)
7.    {
8.         item.debugSelectedPrint();
9.         break;
10.    }
11. }
12.  

Of course, this is more towards C# pseudocode. But it should be adaptable to Javascript (read: not Java!) in some way or another.

 

Sam, just to re-iterate something, as I think I understand your angle. The 'else if' statement, is part of the 'if statement family'. I understand you're saying it's 'basically the same as what you're doing', but to be honest, you know you've written it wrong. If else statements are supposed to be super simple, and this one is confusing you.

The layout is similar, but written in a much cleaner way:

Code (CSharp):

1.             if (details == item1)
2.             {
3.                 Debug.Log("1 is selected");
4.             }
5.             else {
6.                 if (details == item2)
7.                 {
8.                     Debug.Log("2 is selected");
9.                 }
10.                 else {
11.                     if (details == item3)
12.                     {
13.                         Debug.Log("3 is selected");
14.                     }
15.                     else {
16.                         if (details == item4)
17.                         {
18.                             Debug.Log("4 is selected");
19.                         }
20.                         else {
21.                             if (details == item5)
22.                             {
23.                                 Debug.Log("5 is selected");
24.                             }
25.                         }
26.                     }
27.                 }
28.             }
29.  
30.             if (details == item1)
31.             {
32.                 Debug.Log("1 is selected");
33.             }
34.             else if(details == item2)
35.             {
36.                 Debug.Log("2 is selected");
37.             }
38.             else if (details == item3)
39.             {
40.                 Debug.Log("3 is selected");
41.             }
42.             else if (details == item4)
43.             {
44.                 Debug.Log("4 is selected");
45.             }
46.             else if (details == item5)
47.             {
48.                 Debug.Log("5 is selected");
49.             }
50.             else
51.             {
52.                 Debug.Log("None of these apply!");
53.             }

The code on the second example does two things. Firstly it's cleaner so you can actually read it quickly, and secondly it forces only one of those conditions to be called. So it will not call multiple conditions.

With the code you've written itself. There is nothing in that code, which would be causing a problem (atleast within it's own logic). Your issue is elsewhere, but I would suggest you re-write those if statements.

 

You wouldn't even need all that. There's no reason to compare details with anything, as all the correct information should be in that class. So we just need to call debugSelectedPrint() on details.

 

Oh damn, I missed the part where you were writing in JavaScript (Which, btw, isn't the same as Java). Disregard my advice, then, I don't know how to do correct inheritance in JavaScript.

EDIT: Learn C#.

 

@aer0ace posted the cleanest format to reduce the nesting.

Code (csharp):

1.     {
2.         if (Details == Item1) Debug.Log("Item 1");
3.         else if (Details == Item2) Debug.Log("Item 2");
4.         else if (Details == Item3) Debug.Log("Item 3");
5.         else if (Details == Item4) Debug.Log("Item 4");
6.         else if (Details == Item5) Debug.Log("Item 5");
7.     }

So you say "is it the first thing?" and if it isnt, then it tries the next else which is an if, then if that fails it tries the next and so on. It's kind of the same way a switch would do it but you cant use one here (as pointed out) so else ifs are the 'cleanest' option.

I guess to take it any further some more context is needed, right?

 

Disregarding the code formatting or whether or not this is the best way to do what you want - how many objects do you have this script on? There are a couple of obvious ways this can print out different values:

- The value of "details" is changing (or the values of item1..item5 are changing)
- The script exists on multiple objects

Before insisting it's only on a single object (as I have done myself before), add "this.GetHashCode()" to your log statements. If the script is executing on multiple objects you'll get multiple values for the hash code, otherwise you should see only a single value.

Otherwise, something is changing the value of "details". Add log messages to track that or use the debugger to step through the code.

 

What is this details object? I suppose its some sort of custom class you wrote? Also, Nested if else? Really?
You could've used an if else_if instead..

 

That's still nested - lack of braces and indentation doesn't change that.

 

c'mon guy's, we all know this is the best way to write this code

Code (csharp):

1.  
2. Debug.Log(Details == Item1 ? "Item1" : Details == Item2 ? "Item2" : Details == Item3 ? "Item3" : Details == Item4 ? "Item4" : "Item5");
3.  

 

There are a lot of good notes about formatting in this thread, but despite your bad formatting, the code you've provided is logically sound.

To answer the crux of your question, your code is going crazy as a result of a problem somewhere else. What types of objects are 'details' at 'item1'?

What is the code that tells 'item1' to be selected?

 

Yes, sorry about that, I started writing the script a while ago before I knew what I was doing really, and I am too lazy to go respace 400 lines of code now, so I'm just rolling with it on this script lol xD

 

Both MonoDevelop and VisualStudio have auto formatting options...

 

Ok, too much to reply too, so I'll cover as much as I can.

1. I know the formatting is terrible, but that is not my issue...
2. the details variable is a variable GUISkin. I know GUI is unconventional now.
3. I tried else if rather than if statements and else statements. It did not work, so I typed all of the logic out rather than using else if. That last resort is what I posted in OP.
4. The variable details is changed in the OnGUI function, as is this section of code. However, this section of code runs BEFORE the details changes.

 

These are the two actual attempts at dealing with your issue beyond just formatting...

http://forum.unity3d.com/threads/if-else-if-else-if-else-if-else-wait-what.378147/#post-2453784
http://forum.unity3d.com/threads/if-else-if-else-if-else-if-else-wait-what.378147/#post-2454142

 

That changes things. Quite significantly. OnGUI gets called multiple times per frame, under different circumstances. How about posting all of the relevant code?

 

How do you know that item1, item2, and item3 aren't constants?

 

details is a GUISkin. You can't make a GUISkin constant.

 

ok, the code is below. I know that the indentation is weird, there are a lot of dangling and useless and irelavent variables and functions and other stuff that does other things. And yes I know this culd be made immensely more simple now with the new UI. Keep in mind I wrote most of this script almost a year ago.

There is also another script (which is basically an edited slightly different version of this) that goes along with this one, however, it isn't very relavant to my issue. Stuff starts going crazy on line 140.

P.S. I have been messing around trying to figure out what exactly is going wrong. What I have found is this: When details SHOULD = 1, it prints 1. 2 and 3 work fine too. However, when I get to 4, it prints out 3, and when I get to five, it prints out 2. This is always the case, so I do not think it has to do with updates (OnGUI) and stuff not running fast enough to update different things, because then I would expect the results to be very sporadic.

Code (csharp):

1.  
2. #pragma strict
3.  
4. private var isDoor : boolean = false;
5. private var isCrate : boolean = false;
6. private var inventoryOn : boolean = false;
7. private var itemSelected : boolean = false;
8.  
9. var interactButton : GUISkin;
10. var defaultSkin : GUISkin;
11. var inventorySkin : GUISkin;
12. var inventorySkin2 : GUISkin;
13. var exitSkin : GUISkin;
14.  
15. var mainCam : Camera;
16. var aimCam : Camera;
17. var ThirdCam : Camera;
18. var camDistance : int;
19.  
20. private var pages = 1.0f;
21. private var doorScene : int;
22. private var currentPage : int;
23. private var items = 1.0f;
24.  
25. private var num1 : int;
26. private var num2 : int;
27. private var num3 : int;
28. private var num4 : int;
29. private var num5 : int;
30.  
31. private var details : GUISkin;
32. private var item1 : GUISkin;
33. private var item2 : GUISkin;
34. private var item3 : GUISkin;
35. private var item4 : GUISkin;
36. private var item5 : GUISkin;
37.  
38. private var crate : GameObject;
39. private var door : GameObject;
40.  
41. function OnTriggerEnter (other : Collider) {
42. if (other.gameObject.tag == "Door") {
43. door = other.gameObject;
44. doorScene = other.gameObject.GetComponent.<Door>().nextScene;
45. isDoor = true;
46. }
47.  
48. if (other.gameObject.tag == "Crate") {
49. details = defaultSkin;
50. isCrate = true;
51. crate = other.gameObject;
52. items = other.gameObject.transform.childCount;
53. pages = items/5;
54. pages = Mathf.Ceil(pages);
55.   if (pages == 0) {
56.   pages = 1;
57.   }
58. currentPage = 1;
59. }
60. }
61.  
62. function OnTriggerExit (other : Collider) {
63. if (other.gameObject.tag == "Door") {
64. isDoor = false;
65. }
66.  
67. if (other.gameObject.tag == "Crate") {
68. isCrate = false;
69. }
70. }
71.  
72. function OnGUI () {
73. if (isDoor == true) {
74.   GUI.skin = interactButton;
75.   if (GUI.Button(Rect(Screen.width/1.97 - (Screen.width/10)/2, Screen.height/1.24 - (Screen.height/10)/2, Screen.width/12, Screen.height/9), "")) {
76.   PlayerPrefs.SetInt("PlayerX", door.GetComponent.<Door>().X);
77.   PlayerPrefs.SetInt("PlayerY", door.GetComponent.<Door>().Y);
78.   PlayerPrefs.SetInt("PlayerZ", door.GetComponent.<Door>().Z);
79.   PlayerPrefs.SetInt("PlayerRotY", door.GetComponent.<Door>().RotY);
80.   PlayerPrefs.SetInt("Day", GameObject.FindGameObjectWithTag("SunSphere").GetComponent.<GameTime>().GameDay);
81.   PlayerPrefs.SetInt("Hour", GameObject.FindGameObjectWithTag("SunSphere").GetComponent.<GameTime>().GameHour);
82.   PlayerPrefs.SetInt("Minute", GameObject.FindGameObjectWithTag("SunSphere").GetComponent.<GameTime>().GameMinute);
83.   Application.LoadLevel(doorScene);
84.   }
85. }
86.  
87. if (isCrate == true) {
88.   GUI.skin = interactButton;
89.   if (GUI.Button(Rect(Screen.width/1.97 - (Screen.width/10)/2, Screen.height/1.24 - (Screen.height/10)/2, Screen.width/12, Screen.height/9), "")) {
90.   inventoryOn = true;
91.   }
92. }
93.  
94. if (inventoryOn == true) {
95. mainCam.farClipPlane = 0.01;
96. aimCam.farClipPlane = 0.01;
97. ThirdCam.farClipPlane = 0.01;
98. isCrate = false;
99. GUI.skin = inventorySkin;
100. Time.timeScale = 0;
101.  
102. //create inventory
103. GUI.Box(Rect(Screen.width/20 - (Screen.width/10)/2, Screen.height/20 - (Screen.height/10)/2, Screen.width/1, Screen.height/1), "");
104. GUI.Box(Rect(Screen.width/9 - (Screen.width/10)/2, Screen.height/4.9 - (Screen.height/10)/2, Screen.width/4.8, Screen.height/1.5), "");
105. GUI.Box(Rect(Screen.width/1.286 - (Screen.width/10)/2, Screen.height/4.9 - (Screen.height/10)/2, Screen.width/4.8, Screen.height/1.5), "");
106. GUI.Box(Rect(Screen.width/5.4 - (Screen.width/10)/2, Screen.height/9 - (Screen.height/10)/2, Screen.width/15, Screen.height/20), currentPage + "/" + pages);
107.  
108.   //next page
109.   if (currentPage < pages) {
110.    if (GUI.Button(Rect(Screen.width/4.25 - (Screen.width/10)/2, Screen.height/1.13 - (Screen.height/10)/2, Screen.width/12, Screen.height/9), "")) {
111.    currentPage = currentPage + 1;
112.    itemSelected = false;
113.    }
114.   }
115. //exit button
116. GUI.skin = exitSkin;
117.   if (GUI.Button(Rect(Screen.width/2 - (Screen.width/10)/2, Screen.height/1.13 - (Screen.height/10)/2, Screen.width/12, Screen.height/9), "")) {
118.   inventoryOn = false;
119.   itemSelected = false;
120.   Time.timeScale = 1;
121.   mainCam.farClipPlane = camDistance;
122.   aimCam.farClipPlane = camDistance;
123.   ThirdCam.farClipPlane = camDistance;
124.   }
125.  
126. //previous page
127.   if (currentPage > 1) {
128.   GUI.skin = inventorySkin2;
129.    if (GUI.Button(Rect(Screen.width/9 - (Screen.width/10)/2, Screen.height/1.13 - (Screen.height/10)/2, Screen.width/12, Screen.height/9), "")) {
130.    currentPage = currentPage - 1;
131.    itemSelected = false;
132.    }
133.   }
134.  
135. //details
136. if (itemSelected == true) {
137.   GUI.skin = details;
138.   GUI.Box(Rect(Screen.width/2.65 - (Screen.width/10)/2, Screen.height/3 - (Screen.height/10)/2, Screen.width/3, Screen.height/2), "");
139.  
140.   //add to inventory button
141.   GUI.skin = defaultSkin;
142.   if (GUI.Button(Rect(Screen.width/2 - (Screen.width/10)/2, Screen.height/5 - (Screen.height/10)/2, Screen.width/12, Screen.height/9), "Take")) {
143.    if (details == item1) {
144.     Debug.Log("1 is selected");
145.    }
146.    else {
147.     if (details == item2) {
148.      Debug.Log("2 is selected");
149.     }
150.     else {
151.      if (details == item3) {
152.       Debug.Log("3 is selected");
153.      }
154.      else {
155.       if (details == item4) {
156.        Debug.Log("4 is selected");
157.       }
158.       else {
159.        Debug.Log("5 is selected");
160.       }
161.      }
162.     }
163.    }
164.   }
165. }
166.  
167. //create the items per page
168. //item 1
169. num1 = (currentPage * 5) - 5;
170. if (crate.gameObject.transform.childCount > num1) {
171. item1 = crate.gameObject.transform.GetChild(num1).GetComponent.<weapon>().icon;
172. }
173. else {
174. item1 = defaultSkin;
175. }
176. GUI.skin = item1;
177.   if (GUI.Button(Rect(Screen.width/8.6 - (Screen.width/10)/2, Screen.height/4.4 - (Screen.height/10)/2, Screen.width/5.1, Screen.height/8.4), "")) {
178.   details = item1;
179.   itemSelected = true;
180.   }
181.  
182.  
183. //item 2
184. num2 = (currentPage * 5) - 4;
185. if (crate.gameObject.transform.childCount > num2) {
186. item2 = crate.gameObject.transform.GetChild(num2).GetComponent.<weapon>().icon;
187. }
188. else {
189. item2 = defaultSkin;
190. }
191. GUI.skin = item2;
192.   if (GUI.Button(Rect(Screen.width/8.6 - (Screen.width/10)/2, Screen.height/2.85 - (Screen.height/10)/2, Screen.width/5.1, Screen.height/8.4), "")) {
193.   details = item2;
194.   itemSelected = true;
195.   }
196. //item 3
197. num3 = (currentPage * 5) - 3;
198. if (crate.gameObject.transform.childCount > num3) {
199. item3 = crate.gameObject.transform.GetChild(num3).GetComponent.<weapon>().icon;
200. }
201. else {
202. item3 = defaultSkin;
203. }
204. GUI.skin = item3;
205.   if (GUI.Button(Rect(Screen.width/8.6 - (Screen.width/10)/2, Screen.height/2.105 - (Screen.height/10)/2, Screen.width/5.1, Screen.height/8.4), "")) {
206.   details = item3;
207.   itemSelected = true;
208.   }
209. //item4
210. num4 = (currentPage * 5) - 2;
211. if (crate.gameObject.transform.childCount > num4) {
212. item4 = crate.gameObject.transform.GetChild(num4).GetComponent.<weapon>().icon;
213. }
214. else {
215. item4 = defaultSkin;
216. }
217. GUI.skin = item4;
218.   if (GUI.Button(Rect(Screen.width/8.6 - (Screen.width/10)/2, Screen.height/1.665 - (Screen.height/10)/2, Screen.width/5.1, Screen.height/8.4), "")) {
219.   details = item4;
220.   itemSelected = true;
221.   }
222. //item5
223. num5 = (currentPage * 5) - 1;
224. if (crate.gameObject.transform.childCount > num5) {
225. item5 = crate.gameObject.transform.GetChild(num5).GetComponent.<weapon>().icon;
226. }
227. else {
228. item5 = defaultSkin;
229. }
230. GUI.skin = item5;
231.   if (GUI.Button(Rect(Screen.width/8.6 - (Screen.width/10)/2, Screen.height/1.38 - (Screen.height/10)/2, Screen.width/5.1, Screen.height/8.4), "")) {
232.   details = item5;
233.   itemSelected = true;
234.   }
235. }
236. }

 

Update: Instead of if else statements, I am going to make isSelected a var int, and I will use its value instead. I will report whether it works or not.

EDIT: Ok, I changed up my script a bit, ended up make isSelected a Transform, and to make a very complicated thing short, got everything to work fine without needing confusing else if statements!

 

Shouldn't it be something like

Code (CSharp):

1. if (itemSelected == true)
2. {
3.   GUI.skin = details;
4.   GUI.Box(Rect(Screen.width/2.65 - (Screen.width/10)/2, Screen.height/3 - (Screen.height/10)/2, Screen.width/3, Screen.height/2), "");
5. if (GUI.Button(Rect(Screen.width/2 - (Screen.width/10)/2, Screen.height/5 - (Screen.height/10)/2, Screen.width/12, Screen.height/9), "Take"))
6. {
7.  
8. if (details == item1)
9. {
10.     Debug.Log("1 is selected");
11. }
12. else if (details == item2)
13. {
14. Debug.Log("2 is selected");
15. }
16. else if (details == item3)
17. {
18. Debug.Log("3 is selected");
19. }
20. else if (details == item4)
21. {
22. Debug.Log("4 is selected");
23. }
24. else
25. {
26. Debug.Log("5 is selected");
27. }
28.  
29.  
30. } // end of itemselect if

instead of having if statements within else?

 

Honestly, you might be better off redoing the whole thing. It seems like its a great place to use a list or an array instead of all your unique named variables. That will give you cleaner, extensible and maintainable logic.

Rewriting is probably going to be less effort then continuing to maintain broken code.

 

Well, I guess they could have defined constants with those names.

But those would be some weird names for constants.

 

Try it yourself.

Code (csharp):

1. const GUISkin test = null;

 

Yes, because all 3 of those GUISkins are all 'null'...

 

How about that. I always thought const's had to be literals.

Anyway, technically true, but it's not a solution to this problem. I'd say @BoredMormon is correct. Start from scratch.

 

This is the best example if you are doing multiple items it's always best to use an array.
However, I highly recommend for instead of foreach.

if you do still need to single items out then a switch statement is the alternative instead of all the if/else if's.

 

Both MonoDevelop and Visual Studio can automatically reformat your code for you.

http://stackoverflow.com/questions/10457236/format-code-in-monodevelop/15820703#15820703
http://stackoverflow.com/questions/...-format-code-in-visual-studio/5755979#5755979

 

Doesn't help the forum, when you paste it here it gets bad formatting automatically.

 

Not if you use the code tags

 

It's not nested, it's cascaded. They're most prolly compiled to the same assembly/bytecode but to a human, nested ifs' are a mess to read. cascaded ifs' are more comfortable to the eye and the brain. Readability counts, right?

And to the OP, hope it's been fixed by now