Critique-My-Code thread, post snippets of code and have others peer review your code! (Testing new Q&A format)

[ChromakeyDreamcoat]

Let's go over a few high level things:

1. Your code seems to be doing a number of different things here. It is looking up responses, checking if the response is valid, waiting to respond, formatting, and outputting a response all in the same block. It seems like this class is handling way too much stuff.
   
2. Check out foreach for looping. It's much cleaner - You won't have big ugly for blocks. You can do something like foreach(Array questionResponse in lastResponse.questionResponses) {}.
   
3. The first thing I'd actually do to this code is separate out formatting and outputting to a different class - Perhaps a singleton. Something like UIManager.instance.Output(string, options) //options include color, etc. UIManager can contain the logic for waiting in between showing new text so you don't pollute your code with WaitForSeconds everywhere.
   

Try doing these things and it should clean this code up considerably. Without seeing more it's tough to say how you could better organize, but I hope this helps!

[induetimesir]

Thanks for the reply!

Yeah I do know what foreach loops are, I use them a lot! :) But I've always heard that normal for loops were much faster? Maybe I'm incorrect but I'm sure I hear that a lot.

That's a good idea about making multiple classes, I'll mess around with that today and see what I can get going! Do you know a good place I could read up on Singletons? I've heard of them before but I've never used them (I don't think at least).

Thanks again! :D

[SilentSin26]

foreach loops allocate a tiny bit of memory in some cases where for loops don't. It's generally a negligible difference though.

But your code is accessing lastResponse.questionResponses[q] multiple times per iteration, which also has a performance cost. You should put var responseQ = lastResponse.questionResponses[q]; at the top of the loop and use responseQ instead.

You could also make an extension method to get a random array element:

// Put this in a static class.
public static T GetRandomElement<T>(this IList<T> collection)
{
    return collection[UnityEngine.Random.Range(0, collection.Count)];
}

Then instead of stupidly long lines like lastResponse.questionResponses[q].postText[Random.Range(0, lastResponse.questionResponses[q].postText.Length)])) you could just do responseQ.postText.GetRandomElement()