Code Review

[random spruce]

Hi, please check out this script. I would appreciate concrete steps and feedback as to what I can do to improve the code and its structure. Its still an early version *(ignore the playerprefs, I will replace that with a standard saving system later on) *so figured better now than later.

https://gist.github.com/adamsbajgar/1000ba0a734c5de5035dc2c017d87175

Thanks.

@full widget

[dapper zodiac]

you are doing this

 yield return new WaitForSeconds(1);

twice inside while loops.
it is more efficient to create a variable for this

WaitForSeconds wfs = new WaitForSeconds(1);
...
while {
   yield return wfs;
}

[full widget]

• Private fields should start with an underscore
• Methods should be PascalCase, and not start with a lower character

I generally advice you explicitly specify private. For example:

- void Start()
+ private void Start()

[random spruce]

I was taught to always use camelCase except for Classes. Is that wrong?

[full widget]

Yes

This is a javascript thing

In C# you use PascalCase

[random spruce]

Ay okay my fault, I thought that these sort of things translated

[dapper zodiac]

there is no such thing as 'wrong' when it comes to style. Consistency is the only thing which is important

[full widget]

No

But I understand you might be confused because Unity does it

It's an old and horrible convention from back when they had their own scripting language

These conventions also make it so the code is readable. Underscores for private members and such make it clear what something is instead of having to figure it out

Also just generally simplifications of code help a lot with readability:

public void shareConnectedStatus(bool connectedStatus)
{
-    if (connectedStatus == true)
-    {
-        statusMessage.text = "Status: Connected";
-        authContinueButton.SetActive(true);
-        reconnectButton.SetActive(true);
-    }
-    else
-    {
-        statusMessage.text = "Status: Disconnected";
-        reconnectButton.SetActive(true);
-    }

+    statusMessage.text = connectedStatus ? "Status: Connected" : "Status: Disconnected";
+    reconnectButton.SetActive(!connectedStatus);
+    authContinueButton.SetActive(connectedStatus);
}

I assume this code is wrong in general since the buttons are not set active like I expect them to be?

Like, the reconnect button seems logical to be active when you're disconnected, and not both?

[random spruce]

That is a good point,you are right, I dont know why I did that.

[random spruce]

Never knew that was a thing, look at me learning history in a code review (:

I also had somebody say that it does too much based on thisGameStatus, is that right? If so, should I split activateWindow into smaller functions?

[full widget]

What is thisGameStatus?

Some system that sets data depending on what window is open?

Generally you should keep things declarative and make separate things depending on a window in this case, and not a system where some handler method sets it based on the state

In this case it looks like you should have separate window handlers or something

[random spruce]

I am just making myself food, I will be back soon and respond.

[plain berry]

This class seems to be doing a ton. I didnt go through all the logic but parts of this seems like it can be split up.

Firstly shuffleArray can be made into an extension method, working with any IList, removing it entirely from this file.

2nd thing I notice, answerButtons[whatever] is pretty much only used in combination with GetComponent or GetComponentInChildren. It doesnt seem like you actually need this current script to store the button. The object with the button should have its own script on it, that holds a reference to its button, TMP_Text, and image, and then directly change whats needed.
Example:

- answerButtons[correctAnswer].GetComponent<Image>().color = new Color(0, 150, 0, 255);
+ answerButtons[correctAnswer].ChangeColor(whatever Color here)

And answerButtons would be an array of your own custom class which has a ChangeColor method.

The GetComponents there can be avoided entirely. it wont be a performance killer in this case, but you just dont need it at all.
void activateWindow is another example of something doing a lot of logic, its not really even clear why it does so much given the name implies its just activating a window.

I dont really like to talk much about styling though i will say the main thing you should change is make your methods PascalCase. Literally everybody else using c# will have pascal case methods but tbh I dont even use underscores or explicitly write private. As steve said, its all about consistency. If you're in a team where they just dont use underscore then you dont wanna just force it.
Your code also isnt consistent with private. Lines 52-54 dont have it written, along with half the methods

[full widget]

I generally advice you do follow conventions since a modern team will expect them, but feel free to do your own style ofc

But if you don't have a reason for it, maybe look into using them

Convention does change quite often. Not too long ago you'd prefix member fields with m_

Pretty sure some of these still apply and nobody uses them

[random spruce]

Yep, I will change to Pascal and work with the conventions from now on. I just was using camelCase up to this point, without realizing that it is not used here.

[modern sundial]

there are also lots of "magic values" of all kinds, strings, colors, numbers
you can make most of those values serializable and get a lot more flexibility, especially with timers and score values

style-wise, comparing bool values with true or false is redundant and should be avoided

[plain berry]

ah yea i was literally about to write about the magic values in my example above after saying whatever Color here. some reason forgot to include it

[random spruce]

So magic values are poorly explained variables that are essential to the codes functionality? I never came across this terminology before, and that is what google says.

If so, how do I get around this? I am assuming that making them serializable does not fix the core issue here.

*which I am assuming is that they have little context or explanation

[plain berry]

making them serializable does fix the issue. you can still set default values and use the inspector. right now you'd need to recompile the game to change the color through code, rather than being able to do it even at runtime with inspector

in this case its not really about it being poorly explained, although that does apply in other cases.

[modern sundial]

you can go a few ways about it:

• turning them into constants
• making them serializable
• delegating them to another service, like localization

it makes your code more readable because it forces you to give proper names to things instead of assuming prior knowledge regarding the values
and depending on the solution you pick, it also decouples the code logic from the configuration, which is almost always a good thing to do 👍

[plain berry]

The "poorly explained" variables are often because beginners will do this

Vector3 force = whatever;
// later on in some method
rb.AddForce(force * 50000f * Time.deltaTime);

this is a magic value, which is often related to their setup of a rb or misuse of delta time.

but its just easier to say magic values for what you have too, in which case it means just serialize it so you can adjust it easily and faster

[random spruce]

The reason I did not apply [SerializeField] to more variables is that I thought it would be better if there were less of them to clutter the inspector, when I am not likely to adjust most of them.

[random spruce]

How would turning them into constants help?

[random spruce]

So is it generally better to just expose all/most of the variables, even if you are unlikely to adjust them, to make the script more flexible, at the cost of making the inspector more cluttered?

[random spruce]

Basically, its a function that activates the correct window and then does the necessary "setup" functionality, like picking and displaying random sentences to be displayed, etc.

I am assuming that it would be much better to create a function that will activate the correct window, and then have seperate functions for each window functionality?

[plain berry]

for most of the built in types (int, float, vector, color) you can probably just do it yea. if you're populating the values via some function then obviously you wouldnt need to.
also there are many ways to organize your components. this is kinda in the editor scripting department but there are built in assets which really help organize too. https://github.com/dbrizov/NaughtyAttributes this is a pretty common one, which has the Foldout and ShowIf / HideIf attributes. Also Header attributes are a decent way to organize

remember that "unlikely to adjust them" means exiting playmode, recompiling, entering playmode just to change a value through code.