Cleaning code up so it isn't so long?

I've gotten to the point where I can make a functional piece of code, but I don't think my method is the most efficient it could be. For example, say I want to make a phone in-game. I would usually get the images,

Code (CSharp):

1. public Image tablet, cameraicon, flashlighticon, messageicon, remindersicon, settingsicon;

and then disable them so they don't show on the canvas unless I want them to.

Code (CSharp):

1. void Start() {
2.         tablet.enabled = false;
3.         cameraicon.enabled = false;
4.         flashlighticon.enabled = false;
5.         messageicon.enabled = false;
6.         remindersicon.enabled = false;
7.         settingsicon.enabled = false;
8.     }
9.  

When I want to disable certain images, I would do so by re-using the code you see above. This makes the code very overpopulated and I don't like the look of it. My primary questions are: Where would I go to find a more efficient way to write code and what can I do to improve the code you see above? If there is a more elegant way to write it, let me know. Anything you can provide would benefit me. Thank you!

 

Wrap it all in a single method. Then put the method into a region and fold it closed so you don't have to look at it.

 

Definitely possible, but the reason I am asking is because when I present the project I am working on I need to provide source code. Obviously, they will want to see the whole thing, and I want to make it as brief as possible and still be functional.

 

Since they are all images you can put them in an array or list and iterate over that.

I'm not 100% sure this makes your code more readable, it depends what else you use the references for.

Or you can disable a parent object that holds all of the objects.

 

You could also split your class into smaller classes, where each one would responsible for handling a part of your UI. For instance a class for controlling the camera, another one for the the flash light, one for the messages, ...

 

A way to do it would be the following:

Code (CSharp):

1. using System.Collections.Generic;
2. using UnityEngine;
3. using UnityEngine.UI;
4.  
5. public class testscript : MonoBehaviour
6. {
7.     public List<Image> images;
8.  
9.     // Use this for initialization
10.     private void Start()
11.     {
12.         if (images != null &&
13.             images.Count > 0)
14.         {
15.             foreach (var image in images)
16.             {
17.                 ToggleImageEnabled(image);
18.             }
19.         }
20.     }
21.  
22.     /// <summary>
23.     /// Toggles the image enabled state.
24.     /// </summary>
25.     /// <param name="image">The image.</param>
26.     private static void ToggleImageEnabled(Image image)
27.     {
28.         image.enabled = !image.enabled;
29.     }
30. }
31.  

In this script it is assumed that the images initial state is always enabled, so it will disable them. Any disabled images will obviously be enabled.

 

This is more along the lines of what I was looking for. I'll do some research on it all. Thanks for the help!

 

Another little thing you can do if you have more scripts you want to enable/disable, is create a static Utility class and then define the ToggleImageEnabled there, call it ToggleBehaviourEnabled and use the parameter 'Behaviour behaviour' instead of the Image. Then all components that extent Behaviour can be enabled and disabled with the same function, including the Image component!

Going even a step further you can also make it a extension method:

Code (CSharp):

1. public static class BehaviourExtensions
2. {
3.     public static void ToggleEnabled(this Behaviour behaviour)
4.     {
5.         behaviour.enabled = !behaviour.enabled;
6.     }
7. }
8.  
9. // Sample uage
10. Image image = GetComponent<Image>();
11. image.ToggleEnabled();

 

Also if the list is a static length you could use 'arrays' instead of lists and 'for loop' instead of foreach for better performance. You could then make the code more readable by using enums to define the elements in the array.

 

Just to add to the conversation.
(Dont really know what I am adding, merely re-enforcing already posted ideas)
(NOTE: These are untested and Unity was never opened!)

Heres a utility class:

Code (CSharp):

1. using System.Collections;
2. using UnityEngine;
3. using UnityEngine.UI;
4.  
5. namespace Utility {
6.     public class Utilities {
7.      
8.         public static void DisableImages(Image[] images)
9.         {
10.             foreach(var image in images)
11.                 image.enabled = false;
12.         }
13.      
14.         public static void EnableImages(Image[] images)
15.         {
16.             foreach(var image in images)
17.                 image.enabled = true;
18.         }
19.     }
20. }

And heres the script you could use for your UI.

Code (CSharp):

1. using System.Collections;
2. using UnityEngine;
3. using UnityEngine.UI;
4.  
5. public class YourUIScriptName : Monobehaviour{
6.     public Image[] yourImages;
7.  
8.     void Start(){
9.         DisableImages();
10.     }
11.  
12.     void DisableImages(){
13.         Utility.Utilities.DisableImages(yourImages);
14.     }
15. }

 

that just seems like a lot of extra clutter, long code doesn't == better code.
if it is really that important for the code to be short, instead of readable or fast just use linq.
if its a List<Image> you can do this.

Code (CSharp):

1. images.ForEach(x => x.enabled = false);
2. images.ForEach(x => x.enabled = true);

Now back to reality just write your code in the most readable way for yourself, no one really cares what it looks like as long as it does what it is supposed to do and can easily be understood.

 

All these options. And honestly, I would still go with the OP's code.

Shorter code does not mean better code.

 

He's right. On my first University year I had to make a website that made heavy use of JavaScript (it was a web-programming class). During the examination day I was in front of the professor, who was checking my website and he told me I misspelled some words but the site was okay. When I asked if he looked at the code, he simply answered "It works, that's what code should do". Moral of the story, unless your need to optimize every single possible computation, don't care too much about the code, as long as you can understand it.

 

I hope he did make sure you understood the concept of what you programmed, otherwise it's quite a terrible teacher.
Apart from that code should obviously work, it should also be readable and maintainable, not only for yourself but mainly also for others. In any place apart from your own home, the chances someone else will need to do something with your code at some point is pretty big, and thus you should always strife for it. That is the reason patterns and practices exist. For the OP the practice of DRY (Don't Repeat Yourself). It makes sense for the OP to make a single function that can handle toggling the enabled state of the object, although he did give a pretty limited scope.

 

Sometimes it means slower code.

 

Yup, technically ... the OP is the fastest (and less RAM use) method.

Anything else is going to create temporary references, or extra steps internally to change its reference and access point. And I personally love LinQ, but it is unstable on iOS. Myself personally, I hate Apple, so I would use LinQ anyway - but ymmv.

When iterating through things like this in WebGL, keep in mind that the references are not released until the very end of the initiator. So, enough nested iterative functions, and you'll run out of memory. Especially when using strings and earlier versions of Unity (5.2 and earlier).

That said, I'd probably abstract some of it away for readability and flexibility - but technically, performance I think would go to the OP for the win...

 

I would leave it as it is. Lists, Arrays or however you want to do it are fine, but you don't have that many variables in that list and I think it is more readable in it's initial state. It is very possible to try to shorten code, and in many cases it is valid to do, but personally I don't think this is one of those times.

If you are enabling and disabling these objects in several places then this is the way to go... perhaps without the region... I hate regions

 

Looks "nicer" also if rename and re-order those variables : )

Code (CSharp):

1.        
2.         tablet.enabled = false;
3.         iconFlash.enabled = false;
4.         iconCamera.enabled = false;
5.         iconMessage.enabled = false;
6.         iconReminder.enabled = false;
7.         iconSettings.enabled = false;

(could be something else also, without icon-part, or reversed: flashIcon.enabled, or short versions iconSetup, iconCam..)

 

Looking at the bigger picture, you're probably going to need different specific pieces of code to handle camera functionality, messaging, reminders, and all the other things on the list. I'd either make those separate bits of code manage their own icons, or if the icons are (for example) all shown in some sort of shared "container" I'd make the container handle the icons generically, rather than hard-coding each of them, then your separate functionality-handlers would register their icons with the container and ask the container to show or hide the icon as needed.