Looking for suggestions on script.

Hi folks!

Newbie to Unity and C# here, and as such I'm trying to get into C#.
I've been working on this player stats script (outside unity as I don't have it installed on this laptop). And are just wondering if I'm even close to getting it Some code are unfinnished with comments, but you should get the general idea of it. Please give som feedback if I'm on/off track, suggestions to improve would be great.

The part with gameObject is supposed to get info from scripts attached to weapons. I know it's wrong, but haven't gotten around to google how it's coded yet.

Cheers!
Toro

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class PlayerStats : MonoBehaviour
5. {
6.     // Base Stats
7.     public int _curHealth;
8.     public int _maxHealth;
9.     public int _curNano;
10.     public int _maxNano;
11.     public int _curXP;
12.     public int _maxXP;
13.     public int _curLevel;
14.     public int _maxLevel = 50;
15.     public int _abilityPoints;
16.  
17.     // Attributes
18.     public int _strength;   // Melee damage, carry capacity.
19.     public int _stamina;    // Health pool, Endurance.
20.     public int _intellect;  // Nano damage, Nano crit hit chance/damage.
21.     public int _agility;    // Ranged damage, Crit hit chance/damage.
22.  
23.     // Combat Stats
24.     public int _totalDamage;
25.     public int _baseMeleeDamage;
26.     public int _baseNanoDamage;
27.     public int _baseRangeDamage;
28.     public int _critHit;
29.     public int _critChance;
30.     public int _hitChance;
31.  
32.     // Attribute Effects
33.     public int _attribHealth;
34.     public int _attribNano;
35.     public int _attribMeleeDamage;
36.     public int _attribNanoDamage;
37.     public int _attribRangeDamage;
38.     public int _attribMeleeCritHit;
39.     public int _attribNanoCritHit;
40.     public int _attribRangeCritHit;
41.     public int _attribMeleeCritChance;
42.     public int _attribNanoCritChance;
43.     public int _attribRangeCritChance;
44.     public int _attribMeleeHitChance;
45.     public int _attribNanoHitChance;
46.     public int _attribRangeHitChance;
47.  
48.     // Misc
49.     public bool _newPlayer; // Need a system to check if the player has started a new game or loaded a savegame.
50.     public bool _canAttack;
51.  
52.     // Equipped Weapon
53.     public bool _daggerEquipped;
54.     public bool _swordEquipped;
55.     public bool _greatSwordEquipped;
56.     public bool _axeEquipped;
57.     public bool _greatAxeEquipped;
58.     public bool _bowEquipped;
59.     public bool _longBowEquipped;
60.     public bool _nanoDeviceEquipped; // Need to find some sort of "device" as nano equipped weapon...
61.     public bool _pistolEquipped;
62.     public bool _rifleEquipped;
63.     public bool _smgEquipped;
64.  
65.     public gameObject equippedWeapon;
66.  
67.     void Start()
68.     {
69.         if (_newPlayer == true)
70.         {
71.             _curHealth = 80;
72.             _maxHealth = 80;
73.             _curNano = 100;
74.             _maxNano = 100;
75.             _curXP = 0;
76.             _maxXP = 60;
77.             _curLevel = 1;
78.             _strength = 1;
79.             _stamina = 1;
80.             _intellect = 1;
81.             _agility = 1;
82.             _abilityPoints = 0;
83.         }
84.         else
85.         {
86.             // Load in stats from savegame.
87.         }
88.     }
89.  
90.     void Update()
91.     {
92.        
93.  
94.         GainLevel();
95.         UpdateAttributes();
96.         AttributeEffects();
97.         CalculatedDamage();
98.         PlayerDeath();
99.     }
100.  
101.     void PlayerDeath()
102.     {
103.         if (_curHealth < 0)
104.         {
105.             _curHealth = 0;
106.             // Play some death animation and respawn on last save/checkpoint etc...
107.         }
108.     }
109.  
110.     void GainLevel()
111.     {
112.         if(_curXP >= _maxXP)
113.         {
114.             _curLevel++;
115.             _curXP = _curXP - _maxXP;
116.             _maxXP = _maxXP * 2;
117.             _abilityPoints++;
118.             _maxHealth += 25;
119.             _maxNano += 35;
120.             _curHealth = _maxHealth;
121.             _curNano = _maxNano;
122.  
123.             Debug.Log("You have gained a level!");
124.  
125.             UpdateAttributes();
126.         }
127.         else
128.         {
129.             return null;
130.         }
131.     }
132.  
133.     void UpdateAttributes()
134.     {
135.         if(_abilityPoints > 0)
136.         {
137.             // if available attribute points are above 0,
138.             // you can distribute them to you skills of choice.
139.             // This will be done in-game via button actions.
140.  
141.             if () // Strength selected
142.             {
143.                 _strength++;
144.             }
145.  
146.             if () // Stamina selected
147.             {
148.                 _stamina++;
149.             }
150.  
151.             if () // Intellect selected
152.             {
153.                 _intellect++;
154.             }
155.  
156.             if () // Agility selected
157.             {
158.                 _agility++;
159.             }
160.  
161.             Debug.Log("You have gained ability points!");
162.         }
163.         else
164.         {
165.             return null;
166.         }
167.     }
168.  
169.     void AttributeEffects()
170.     {
171.         _attribHealth = _curHealth * _stamina / 100 + equippedWeapon._stamina;
172.         _attribNano = _curNano * _intellect / 100 + equippedWeapon._intellect;
173.         _attribMeleeDamage = _baseMeleeDamage * _strength / 100 + equippedWeapon._strength;
174.         _attribNanoDamage = _baseNanoDamage * _intellect / 100 + equippedWeapon._intellect;
175.         _attribRangeDamage = _baseRangeDamage * _agility / 100 + equippedWeapon._agility;
176.         _attribMeleeCritHit = _critHit * _stamina / 100 + equippedWeapon._stamina;
177.         _attribNanoCritHit = _critHit * _intellect / 100 + equippedWeapon._intellect;
178.         _attribRangeCritHit = _critHit * _agility / 100 + equippedWeapon._agility;
179.         _attribMeleeCritChance = _critChance * _stamina / 100 + equippedWeapon._stamina;
180.         _attribNanoCritChance = _critChance * _intellect / 100 + equippedWeapon._intellect;
181.         _attribRangeCritChance = _critChance * _agility / 100 + equippedWeapon._agility;
182.         _attribMeleeHitChance = _hitChance * _stamina / 100 + equippedWeapon._stamina;
183.         _attribNanoHitChance = _hitChance * _intellect / 100 + equippedWeapon._intellect;
184.         _attribRangeHitChance = _hitChance * _agility / 100 + equippedWeapon._agility;
185.     }
186.  
187.     void CalculatedDamage()
188.     {
189.         if (_swordEquipped == true)
190.         {
191.             _canAttack = true;
192.             _totalDamage = _baseMeleeDamage + _attribMeleeDamage;
193.         }
194.  
195.         if (_greatSwordEquipped == true)
196.         {
197.             _canAttack = true;
198.             _totalDamage = _baseMeleeDamage + _attribMeleeDamage;
199.         }
200.  
201.         if (_axeEquipped == true)
202.         {
203.             _canAttack = true;
204.             _totalDamage = _baseMeleeDamage + _attribMeleeDamage;
205.         }
206.  
207.         if (_greatAxeEquipped == true)
208.         {
209.             _canAttack = true;
210.             _totalDamage = _baseMeleeDamage + _attribMeleeDamage;
211.         }
212.  
213.         if (_daggerEquipped == true)
214.         {
215.             _canAttack = true;
216.             _totalDamage = _baseMeleeDamage + _attribMeleeDamage;
217.         }
218.  
219.         if (_bowEquipped == true)
220.         {
221.             _canAttack = true;
222.             _totalDamage = _baseRangeDamage + _attribRangeDamage;
223.         }
224.  
225.         if (_longBowEquipped == true)
226.         {
227.             _canAttack = true;
228.             _totalDamage = _baseRangeDamage + _attribRangeDamage;
229.         }
230.  
231.         if (_nanoDeviceEquipped == true)
232.         {
233.             _canAttack = true;
234.             _totalDamage = _baseNanoDamage + _attribNanoDamage;
235.         }
236.  
237.         if (_pistolEquipped == true)
238.         {
239.             _canAttack = true;
240.             _totalDamage = _baseRangeDamage + _attribRangeDamage;
241.         }
242.  
243.         if (_riflelEquipped == true)
244.         {
245.             _canAttack = true;
246.             _totalDamage = _baseRangeDamage + _attribRangeDamage;
247.         }
248.  
249.         if (_smgEquipped == true)
250.         {
251.             _canAttack = true;
252.             _totalDamage = _baseRangeDamage + _attribRangeDamage;
253.         }
254.         else
255.         {
256.             canAttack == false;
257.             Debug.Log("You need a weapon to attack. Please equip something!");
258.         }
259.  
260.     }
261. }

 

Hi and welcome!

I'm personally not a huge fan of the underscore convention, but i know a lot of people like it to differentiate between member variables and others. Use it if it helps you, but use it in a meaningful way (see below).

All of your variables are public. Most of the time you do not want this. Generally, most variables should be private. If you want them to show up in the inspector, make them [SerializeField]. If you want them to be gettable / settable create a private variable and a public property (this also allows you to define what else should happen when you set a value, for example call a method or check the set value for negative values and reject it in certain cases and so on). If you actually only want to set a variable from outside this script, then it's fine to put it public. Coming back to the underscore convention, this would be where it would be useful for differentiating between your private member variables (with underscore) and your public properties (without underscore).

In line 65 you wrote 'gameObject' but mean 'GameObject', the type.

You do not need to compare bools to true or false (line 69). If you want to check if _newPlayer is true, if(_newPlayer) already does that, since _newPlayer, as a bool, already contains true or false. If you want to check if it's false, you can write if(!_newPlayer), which reads as "if not _newPlayer", so it's true if _newPlayer is not. This is shorter and for most people easier to read. Especially in longer expressions comparing to true or false will become confusing to most. From a performance perspective, it's an additional unnecessary comparison - even tho realistically this wont have any measurable impact.

In Start() you want to either load in stats or set them to default values. There are two things worth mentioning here. You could just load them (take a look at PlayerPrefs if you have not), and use the values (like 80 for maxHealth) as default values in case no loadable values were found. So if there is no data for a given player, it would instead use these default values, which would be less lines of code and you wouldnt even need the whole if statement, or the newPlayer variable anymore.
Secondly i would save these values in constant attributes. I personally like the capslock-underscore convention to indicate constants, my maxHealth would look something like this: "private const int MAX_HEALTH = 80;". Note that you can directly set the max healht value when you declare the variable, similar to how you did it for maxLevel. Thus you'd also have all these values at one place and could easily change them later on, without having to find them. Having these 'magic numbers' in one place is generally a good idea.

In Update() you just list all kinds of methods. This is not wrong, but i think the method names are a bit misleading. When i call a method named "GainLevel" i would expect my character to go up one level. Instead the method internally checks if the character should go up one level. So first of all, i would do this check on the outside, such that GainLevel() only contains the code that actually increases my level. If you want to wrap this into a method that checks for potential level gains, then i would also call this method something like "CheckAndExecuteLevelUp()".
Secondly, however, i would not check for level-ups every Update cycle. You only gain experience after certain actions, like killing a monster or completing a quest. So these events should then call a public method of the Player, like "GainExperience(int gainedExp)". This method would then increase the exp of the player and internally check if the player has enough exp for a level up (for example by calling CheckAndExecuteLevelUp). Thus you would only call this code when it's actually necessary, and not check for exp / level ups each frame. Executing more expensive operations than this on a frame-to-frame basis, when you dont have to, will lead to a major slowdown of your game. Only do what has to be done, when it has to be done, if possible.

The same goes for taking damage / dying, increasing attribute points and so on. Only call these methods in the events that actually require them, not every frame.

In line 127, you do not need to return anything in a void method. So you do not need to return null. A void method just executes its body. The only reason to return would be if you want to prematurely end the execution of the method. This can be useful, for example, if a required component is null and the method thus cannot execute further without throwing an exception (which can be a legitime situation).

Your weapon system is also a bit questionable - or rather more complicated than it needs to be. Instead of having a bool for every weapon type that can exist in your game and checking if this weapon is equipped, you should have one variable "private Weapon equippedWeapon = ..." where you save the currently equipped weapon. Your damage calculation would then look something like this: "damage = equippedWeapon.damage * someCharacterDamageMultiplier". On that note, i would probably put most of the damage information on the weapon (a custom Weapon type you create, potentially you could use scriptable objects), where it belongs. And unless you really intend to have different crit rates for all weapon types, i would combine these into one, but that's up to your game design.

Hope this helps

Edited: Highlighted some key words for easier overview. I realise people dont want to read over a wall of text, but it's also counter productive if advice is repeated over and over again, so this should make life easier for everyone^^

 

One thing that immediatly strikes my eye, that's your code in

Start()

if it is a new player. Is there any reason not to set the default values in the inspector of the prefab which has this component?

The other thing is how you calculate damage. Hardcoding every weapon type is very tedious and very prone to errors if you copypaste. There are various solutions to this, I recommend you to take a look on ScriptableObjects. Each of the instances of your ScriptableObject represents a weapon type, which knows how its damage is calcluated, through an object method with the stats as parameter, for instance. You would not have to worry about damage calculation in your stats script, because said objects take care of it.

If this is too much at once for you, using an enumeration for weapon types with a switch statement might provide a very readable solution without having to use a lot of IF-THEN-ELIF statements and a bool field for every weapon type.

 

In all, it's very generic, and it may work if you implement the game accordingly. A few notes:

• UpdateAttributes will most probably not the way you have it here; it'll probably have some kind of GUI, and the attributes will probably be increased directly. That's why you weren't able to directly define a guard for strength, Stamina, etc.
• You have lots of attributes, Defining them with individually named variables will work, but incur an immense amount of work if you need to change / refactor. Think about using a Dictionary.
• Think about using scriptable objects to base Player stats on. You then can have 'prefabs' for different classes and enemies.

 

Thanks for awesome feedback! Alot of good points there that I will look into. Thanks for your time

 

Well, besides obvious typos, I will say all those underscores made this hard to read, but I realize that might be a design choice.

And as mentioned, your bool per weapon is a bad design. If you ever add more weapons, you'll have to add another bool. Essentially, it's a hard coded script in a sense due to this.

 

Tbh, the underscore approach was suggestion from a colleague that works as systemdeveloper. I just used that here to find out if it's helpful. For me it doesn't matter much, but obviously not a common design choice

 

I know there was another post somewhere on here that talked about design choices and underscores were part of the topic.

I think a developer's background will determine what they are use to doing, but I don't think it's common in c# to use it. I've been developing for a long time and it isn't something that really saw much. Certainly not with my team at my current job.

For you, if this is personal only, then use whatever convention you want. It may just be a bit distracting if you need help.

 

Unity sometimes uses it to differentiate between member variables and public properties in their scripts, and some people adopted this approach. If you want the variable to be private (_someVariable) and have a public property (SomeVariable), i guess it can become confusing which one you want to work with internally in the script, thus some people use the underscore. However, i'm not a big fan of this myself and personally think writing variables in camelCase and properties in UpperCamelCase would be enough as well.
And as you said, the background often determines the conventions someone uses. So some may use underscores, others may not, and those that do often mean different things with them (constants, members, ...).
It's used very commonly in shader languages tho, to differentiate between variables that can be set from the outside and those that are used internally. There is make quite a bit of sense imho.

 

Except for religion, or perhaps Star Trek vs Star Wars, few topics generate as many flame wars in progrmming forums as 'naming conventions'. -- IMHO: use what you like best and feel helps you most. Be aware that there are many different such schemes, and some make more sense than others; all are designed to help you or your team, and adherance shouldn't be a matter of dogma, but common sense; if you get a job with a larger company, chances are that they will impose some form of naming conventions on to you. Much more important to me is that the names that you choose for a variable are "good" in the sense that even 5 years down the line it is crystal clear to you (and co-workers) what the variable is used for. So avoid "List<Object> list" and choose "List<Object> listOfThingsToDo" instead.