Code Review? Maturity of the code

Would anyone be willing to give my scripts a quick look and share any opinions/best practices I might need to work on?

This is an example of putting together a very simple game with the following features:

List of weapons
List of levels (each containing 10 monsters)
Save/load to binary with a single player prefs string

It's basic and all classes are in a single file - I know for larger games this is confusing/ugly. For this though I did it that way.

 

1) What you do in the constructor could easily be done on the fields themselves. Though you don't do anything bad in it, unity says to avoid using the constructor.

2) rely heavily on default access modifiers, instead of explicit stating private/public. Though not necessary, just my personal preference.

3) Inconsistent naming conventions for methods, with some starting lowercase, and other starting uppercase.

4) Class names are verbose, yet non-descriptive... your class is 'LevelClass', we know it's a class, it's defined as a class. But what is it!? It appears to a container of all the monsters in your level. How about 'MonsterCollection'? Or is this supposed to contain all Level information, and it just happens to be only monsters right? How about 'LevelInfo'?

MonsterClass could just be 'Monster', and WeaponClass could just be 'Weapon'.

5) 'tempStringBinary' and 'tempString' are used as local variables to the functions they're used on. Neither is used to persist any kind of state data about 'GameManager'. Why are they class level fields of GameManager?

6) Speaking of those 2 fields... What is up with the save system of yours? Are you intentionally take a string of 10 or so characters and inflating it to 8 times the length for saving? What is the point of converting it to a string of 1's and 0's?

Also, if you want to get the byte representation of a string, use the Encoding class. It takes care of the dirty work for you:
https://msdn.microsoft.com/en-us/library/ds4kkd55(v=vs.110).aspx
Which you show some knowledge in since you used it to convert back with 'GetString'.

And while you're at it, save the byte array, rather that that fat string.

7) Don't save to PlayerPrefs either... this is probably what led you to store a string since it's really the only fat save type for PlayerPrefs.

And if you really want to use that as the save location... ok, then B64 encode the byte data for the string... not this "1000100011100101000100010111010010000101000001110010010000111000", obtusely sized, save string of yours. Is that an attempt at obfuscating the save data or something?

8) GameManager seems to take care of quite a lot, despite being a little bit of code. This might just be me, as I see a lot of people in Unity hand a lot of jobs to a single script... but usually too many jobs on one class is considered a clue you might be designing yourself into a corner.


Anyways... that's a cursory review.

I didn't vote on your poll as I don't know what 1 or 10 is supposed to represent.

I'll say this, it shows a reasonable understanding of how to program and solve problems and create simple data structures.

But it also shows novice choices.

I will say, you appear more capable than the majority of people I've interviewed for jobs over the years. But this is South Florida, so that's not saying much.

 

Wow that was a great response and so informative! Thanks for taking so much time to look into what I did and offer some pointers. I have a job as a SharePoint Dev, but it is a lot of "Middleware" and configuring, rather than programming. I don't have any code mentor so this is VERY valuable feedback.

 

I've also looked over your code and echo all of @lordofduct's comments. I'll add a few more:

Code (CSharp):

1. MonsterClass m;
2. WeaponClass w;

This is simply bad naming. It provides no context of what "m/w" is outside of this line declaration. Should be renamed at least to

Code (CSharp):

1.  MonsterClass monster

or even better, what the actual use of this Monster instance does. As of right now, I'm still not sure what this is used for(Which I should be able to tell from the line of declaration). I can't find it outside of the constructor.

Code (CSharp):

1. class LevelClass
2. {
3.     //each level has 10 monsters
4.     MonsterClass m = new MonsterClass ();
5.     List<MonsterClass> listofMonsters;
6.  
7.     public LevelClass ()
8.     {
9.         listofMonsters = new List<MonsterClass> ();
10.     }
11.  
12.     public void addMonster (string name, int hp, string platform)
13.     {
14.         m = new MonsterClass ();//clear obj
15.         m.name = name;
16.         m.platform = platform;
17.         m.hp = hp;
18.         listofMonsters.Add (m);
19.     }
20.  
21.     public MonsterClass getMonster (int i)
22.     {
23.         return listofMonsters [i];
24.     }
25.  
26. }

Why do you create the actual monster class inside of the method? I'd consider this a bad practice. Your code should be verbose, and in this case the method isn't following it's naming principle. It is not adding a Monster, it's actually creating the monster and then adding it. Here is how I'd redo this class( keep in mind that I'm following your restraints on keeping the whole solution working).

Code (CSharp):

1. class LevelInfo
2. {
3.     public List<Monster> Monsters { get; private set; }
4.  
5.     public LevelInfo ()
6.     {
7.         Monsters = new List<Monster> ();
8.     }
9.  
10.     public void AddMonster (Monster monsterToAdd)
11.     {
12.         monsters.Add (monsterToAdd);
13.     }
14. }

This greatly simplifies the code, and actually does what it says. You'd then explicitly create the Monster class in the method parameter.


I won't go into anything with the saving/loading of levels as it is examined above.

Code (CSharp):

1. IEnumerator loadWeapons ()
2.     {
3.         yield return new WaitForSeconds (0);
4.         //create the entire list of weapons if it isn't too taxing on performance.
5.         createWeapon ("Fist", 10, 1f);
6.         createWeapon ("Stick", 10, 1f);
7.         createWeapon ("Knife", 10, 1f);
8. }

What are you trying to do here? It simply executes the function on the next frame. Just write it as a normal method, nothing you do here is taxing at all. Also, it seems that you're confused on what a coroutine does. This coroutine will only run once, since it has no looping mechanism to maintain it's continuity.

http://docs.unity3d.com/Manual/Coroutines.html