Advices for clean and easy to understand code

My unity scripts are a bit messy and I have no idea how to make them clean and understandable. When I write something and it works, after 2 weeks I have no idea what each variable does, and how the script works. And things keep breaking in my project, and fixing a glitch causes three more glitches. I really don't want to abandon this project, however I can't seem to keep up with the problems that appear in the code. For example: This is my first script I wrote for this project one and a half years ago(when I knew nothing about game dev) and it is horrible(Now at least I think I might be able to come with a clean script). It was meant to switch between items in inventory and it works, however I have no idea how and it is impossible to add a feature or modify something in this mess

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4. using UnityEngine.UI;
5.  
6. public class item_select : MonoBehaviour {
7.     public GameObject Inventory;
8.     public GameObject Item;
9.     public Shader shader;
10.     public GameObject changed;
11.     public GameObject ItemInHand;
12.     public Vector3 equippos;
13.     public Quaternion equiprot;
14.     public GameObject Equiped;
15.     public GameObject Player;
16.     public GameObject PlayerCamera;
17.     public GameObject air;
18.     public GameObject LastItem;
19.     public GameObject Arm;
20.     public bool here = false;
21.     public GameObject Weapon;
22.     public GameObject ArmsSkin;
23.     public GameObject Outline;
24.     public GameObject Arms;
25.     public GameObject ItemInfo;
26.     public bool isChanged;
27.     public Canvas canvas;
28.     public GameObject LastInfo;
29.     public HungerSystem hunger;
30.     public Image SpyglassImage;
31.  
32.     // Use this for initialization
33.     void Start () {
34.    
35.     }
36.  
37.     // Update is called once per frame
38.     void Update()
39.     {
40.    
41.         if (Inventory.GetComponent<inventory>().InInventory == true){
42.             RaycastHit hit;
43.             Ray ray = Inventory.GetComponent<Camera>().ScreenPointToRay(Input.mousePosition);
44.             if (Physics.Raycast(ray, out hit, 100000.0f)){
45.                 changed = hit.transform.gameObject;
46.                 if (hit.transform.gameObject.tag != "inventory" && changed.tag != "terrain"){
47.                     if(hit.transform.gameObject != Item)
48.                     {
49.                         isChanged = true;
50.                     }
51.                
52.                     //shader = Item.GetComponent<Renderer>().material.shader;
53.                     //Outline.SetActive(true);
54.                 }
55.                 else
56.                 {
57.                     if(LastInfo != null)
58.                     {
59.                         //LastInfo.SetActive(false);
60.                         Item = null;
61.                         Destroy(LastInfo);
62.                         isChanged = true;
63.                     }
64.                 }
65.            
66.  
67.                 if (Input.GetKeyDown(KeyCode.Mouse0))
68.                 {
69.                     if (hit.transform.gameObject.GetComponent<IsWeapon>() != null)
70.                     {
71.                         if (hit.transform.gameObject.tag == ItemInHand.tag)
72.                         {
73.                             //Arms.transform.localPosition = new Vector3(-0.12f, -2.6f, -0.31f);
74.                             Arms.GetComponent<Animator>().Play("IdleAxeArms");
75.                             if (ItemInHand.tag != "Lamp")
76.                             {
77.                                 ItemInHand.GetComponent<MeshRenderer>().enabled = false;
78.                             }
79.                             else
80.                             {
81.                                 ItemInHand.transform.GetChild(0).GetComponent<MeshRenderer>().enabled = false;
82.  
83.                                 ItemInHand.transform.GetChild(1).GetComponent<MeshRenderer>().enabled = false;
84.  
85.                             }
86.                             Arms.GetComponent<Animator>().SetBool(ItemInHand.GetComponent<orange_inventory>().parameter, false);
87.                             //ArmsSkin.GetComponent<SkinnedMeshRenderer>().enabled = false;
88.                             LastItem = ItemInHand;
89.                             ItemInHand = air;
90.                             GetComponent<inventory>().InInventory = false;
91.                             PlayerCamera.SetActive(true);
92.                             Player.GetComponent<UnityStandardAssets.Characters.FirstPerson.FirstPersonController>().enabled = true;
93.  
94.                         }
95.                         else
96.                         {
97.                             //Arms.transform.localPosition = new Vector3(-0.12f, -2.6f, -0.31f);
98.                             if (ItemInHand.GetComponent<pickup_inhand>() != null)
99.                             {
100.                                 ItemInHand.GetComponent<item_drop>().DropItem(ItemInHand);
101.                                 ItemInHand = air;
102.                             }
103.                             if (ItemInHand.GetComponent<pickup_inhand>() == null)
104.                             {
105.                                 if (ItemInHand.tag != "Lamp")
106.                                 {
107.                                     if (ItemInHand.tag != "air")
108.                                     {
109.                                         ItemInHand.GetComponent<MeshRenderer>().enabled = false;
110.                                     }
111.  
112.                                 }
113.                                 else
114.                                 {
115.                                     ItemInHand.transform.GetChild(0).GetComponent<MeshRenderer>().enabled = false;
116.  
117.                                     ItemInHand.transform.GetChild(1).GetComponent<MeshRenderer>().enabled = false;
118.  
119.  
120.                                 }
121.                             }
122.                             if (ItemInHand.tag != "air")
123.                             {
124.                                 Arms.GetComponent<Animator>().SetBool(ItemInHand.GetComponent<orange_inventory>().parameter, false);
125.                             }
126.                             if (ItemInHand.GetComponent<PistolRightClick>() != null)
127.                             {
128.                                 Arms.GetComponent<Animator>().SetBool(ItemInHand.GetComponent<PistolRightClick>().parameter, false);
129.                             }
130.                             ItemInHand = hit.transform.gameObject.GetComponent<orange_inventory>().EquipedWeapon;
131.                             Arms.GetComponent<Animator>().SetBool(ItemInHand.GetComponent<orange_inventory>().parameter, true);
132.  
133.                             if (ItemInHand.tag != "Lamp")
134.                             {
135.  
136.                                 ItemInHand.GetComponent<MeshRenderer>().enabled = true;
137.  
138.  
139.                             }
140.                             else
141.                             {
142.                                 ItemInHand.transform.GetChild(0).GetComponent<MeshRenderer>().enabled = true;
143.  
144.                                 ItemInHand.transform.GetChild(1).GetComponent<MeshRenderer>().enabled = true;
145.  
146.  
147.                             }
148.                             if (ItemInHand.GetComponent<FlintlockShot>() != null)
149.                             {
150.                                 ItemInHand.GetComponent<FlintlockShot>().disabled = true;
151.                             }
152.                             ArmsSkin.GetComponent<SkinnedMeshRenderer>().enabled = true;
153.                             Invoke("ExitInventory", 0.32f);
154.                             PlayerCamera.SetActive(true);
155.                             Player.GetComponent<UnityStandardAssets.Characters.FirstPerson.FirstPersonController>().enabled = true;
156.                             Arms.GetComponent<Animator>().Play("IdleAxeArms");
157.                             Arms.GetComponent<Animator>().SetBool(ItemInHand.GetComponent<orange_inventory>().parameter, true);
158.  
159.                         }
160.  
161.                     }
162.                     if (hit.transform.gameObject.GetComponent<IsEdible>() != null)
163.                     {
164.                         StartCoroutine(HungerSystem.instance.Add(hit.transform.gameObject.GetComponent<IsEdible>().hungerRestore));
165.                         StartCoroutine(ThirstConsume.instance.Add(hit.transform.gameObject.GetComponent<IsEdible>().thirstRestore));
166.                         StartCoroutine(EnergyConsume.instance.Add(hit.transform.gameObject.GetComponent<IsEdible>().energyResotre));
167.                         StartCoroutine(Health.instance.Add(hit.transform.gameObject.GetComponent<IsEdible>().healthResotre));
168.                         GetComponent<inventory_items>().Types[hit.transform.gameObject.GetComponent<orange_inventory>().i]--;
169.                         Destroy(hit.transform.gameObject);
170.                     }
171.                     //if(hit.transform.gameObject.tag == "Spyglass")
172.                     //{
173.                     //GetComponent<inventory>().InInventory = false;
174.                     //PlayerCamera.SetActive(true);
175.                     //Player.GetComponent<UnityStandardAssets.Characters.FirstPerson.FirstPersonController>().enabled = true;
176.                     //Arms.GetComponent<Animator>().SetBool("Spyglass", true);
177.                     //GameObject SpyglassInstance = Instantiate(hit.transform.gameObject, Arm.transform);
178.                     //SpyglassInstance.transform.localPosition = hit.transform.gameObject.GetComponent<equip>().pos;
179.                     //SpyglassInstance.transform.localRotation = Quaternion.Euler(hit.transform.gameObject.GetComponent<equip>().rot);
180.                     //SpyglassInstance.transform.localScale = new Vector3(1, 1, 1);
181.                     //StartCoroutine(Player.GetComponent<SpyglassUse>().SpyglassEnabled(SpyglassInstance));
182.                     //}
183.  
184.                 }
185.            
186.             }
187.             if (isChanged == true && hit.transform.gameObject.tag != "inventory" && changed.tag != "terrain")
188.             {
189.                 if (LastInfo != null)
190.                 {
191.                     //LastInfo.SetActive(false);
192.                     Destroy(LastInfo);
193.                 }
194.                 isChanged = false;
195.  
196.                 GameObject Info = Instantiate(ItemInfo, transform.position, Quaternion.identity, canvas.transform);
197.                 Info.transform.GetChild(0).GetComponent<Text>().text = hit.transform.gameObject.GetComponent<ItemInfoValues>().text;
198.                 Info.transform.GetChild(1).GetComponent<Text>().text = hit.transform.gameObject.GetComponent<ItemInfoValues>().description;
199.                 Info.transform.GetChild(2).transform.GetChild(0).GetComponent<Image>().sprite = hit.transform.gameObject.GetComponent<ItemInfoValues>().Key1;
200.                 Info.transform.GetChild(2).transform.GetChild(1).GetComponent<Text>().text = hit.transform.gameObject.GetComponent<ItemInfoValues>().Action1;
201.                 if (hit.transform.gameObject.GetComponent<ItemInfoValues>().secondAction == true)
202.                 {
203.                     Info.transform.GetChild(3).transform.GetChild(0).GetComponent<Image>().sprite = hit.transform.gameObject.GetComponent<ItemInfoValues>().Key2;
204.                     Info.transform.GetChild(3).transform.GetChild(1).GetComponent<Text>().text = hit.transform.gameObject.GetComponent<ItemInfoValues>().Action2;
205.  
206.                 }
207.  
208.                 Info.transform.GetChild(4).GetComponent<Text>().text = "" + Inventory.GetComponent<inventory_items>().Types[hit.transform.gameObject.GetComponent<orange_inventory>().i];
209.                 Info.transform.position = GameObject.FindGameObjectWithTag("InventoryCamera").GetComponent<Camera>().WorldToScreenPoint(hit.transform.position);
210.                 Item = hit.transform.gameObject;
211.                 LastInfo = Info;
212.             }
213.  
214.         }
215.         else
216.         {
217.             if (LastInfo != null)
218.             {
219.                 Destroy(LastInfo);
220.             }
221.         }
222.  
223.     }
224.     public void ExitInventory()
225.     {
226.         GetComponent<inventory>().InInventory = false;
227.         LastItem = Item;
228.         Item = null;
229.    
230.     }
231.  
232. }
233.  

I think I am able now to replace this horrable mess, however the problem is that many more scripts are similar and it would mean to redo almost the entire project.
Any advices?

 

Volumes could be written... never mind scratch that.. volumes HAVE been written about writing clean code.

Every approach has its pros and cons.

You area actually already FAR AHEAD of the average coder because you have identified that the code above is giving you pain. That's huge! The first step towards fixing something is admitting it is broken.

If I had one super-ultra-generic piece of advice to give it is this: make EVERYTHING have an API that you transact against.

For instance, the above script

item_select

reaches out and manually bangs its way through animators and items and infos and oh my goodness, what a mess.

But it is doing very specific things: selecting an item, displaying info, etc. Those are very specific transactions.

Try to define an API for an Item: what does an Item do? What can be done to an item?

Anytime you work at stuff from an API standpoint what you end up with is separation of concerns. In fact, an Item would probably be a collection of subsystems:

- its definition
- its presentation in the inventory
- its representation in the world
- things that it can do (buffs you, scares enemies, etc)

By keeping all those things separate and having their own API, if you decide to refactor something, as long as you re-make that API and all of its functions, refactoring is a breeze and you can be confident it will still work.

 

For instance, an item can be: Held in hand, used, stored in the bag. It can go from a held in hand state to stored state, from stored state to held in hand and from held in hand to a use state. Making all this states its own API would mean, what exactly?

For example in the item_select script I remember I taught to tell the script what happens when an item is clicked:
if it is already equipped that means click should de-equip
if it is de-equipped that means click should equip it
if another item is equipped, the clicked item should be equipped and the old one should be de-equipped

To have an API work-flow and this case would mean to separate the equip and de-equip instructions in systems, if what I said makes sense? For instance, if the player is holding a flintlock pistion and clicks in the inventory the sword, the animation of the arms should be changed from holding a pistol to holding a sword, then the flintlock renderer should be disabled and the sword renderer should be enabled. Then the inventory should be closed. To separate this things I can create an animation namespace and add

using GameName.Animations 

when a script would deal with animations. Would that make a cleaner code?

What I don't understand quite well is how to define an item for example.
Should I have an object called Item.cs that holds information about an item. Then what information, Should I have an item_select.cs for switching between the items, or equipping and de-equipping are actions of items and should be placed in Item.cs?

Is this a cleaner approach for switching items:

Code (CSharp):

1. void Update(){
2.     if(Raycast hit...){
3.          Item item = hit.transform.gameObject.GetComponent<Item>;
4.          if(item != null){
5.              if(item.name == Player.equipped.name){
6.                   DeequipItem(item.name);
7.              }
8.              else{
9.                   SwitchItem(Player.equipped.name, item.name);
10.              }
11.          }
12.     }
13. }
14. void switchItems(string old_item, string new_item){
15.     foreach(Item item in Player.Items){
16.          
17.            if(old_item == item.name) item.GetComponent<Renderer>().enabled = false;
18.            if(new_item == item.name) {
19.                  item.GetComponent<Renderer>().enabled = true;
20.                  Player.equipped = Item.transform.gameObject;
21.            }
22.            GameName.Animations.SwitchAnimations(old_item, new_item);
23.            CloseInventory();
24.     }
25.  
26. }
27. void deequipItem(string item){
28.        foreach(Item _item in Player.Items){
29.              if(item == _item.name) _item.GetComponent<Renderer>().enabled = false;
30.              Player.equipped = air;
31.        }
32.        GameName.Animations.deequipItem(string item);
33.  
34. }
35.  

However I don't think that is what you meant by API, plus the fact that when de-equipping something you are not holding
any item which can be treated like an item called air, however holding air doesn't have any animation so it would be a special case of item which will mess the code.