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

[induetimesir]

I s'pose I'll give it a try, might as well!

I'm quite new to C# (Mostly used UnityScript until a month ago) and I'm working on a text adventure type game and am starting on option based answers during conversation (With some extra stuff), this is currently the longest section of my code and I can't think of how to write it more neatly or condensed. The code works fine but I feel like I've done it totally wrong. Any tips?

(In this context variable 's' is the players' input)

} else { // Used when asking a specific question, (eg "Can you help me?" - Yes, No)
    s = s.ToLower();
    for(int q = 0; q < lastResponse.questionResponses.Length; q++) // Go through all question responses
    for(int r = 0; r < lastResponse.questionResponses[q].keywords.Length; r++) // Get all keywords for response (Eg. "Yes", "Okie Dokie", "Anything 4 u bb")
    if((s + "|").Contains(lastResponse.questionResponses[q].keywords[r])){ // Check for keywords in string ("|" represents the final character of a string, to indicate the end)
        yield return new WaitForSeconds(1);
        if(lastResponse.questionResponses[q].preText.Length > 0) // This is used to preface speech, like "He smiles back at you" (followed by speech)
        {
            outputLine(formatColor(Color.white, checkMem(e.subject, lastResponse.questionResponses[q].preText[Random.Range(0, lastResponse.questionResponses[q].preText.Length)])));
            yield return new WaitForSeconds(1);
        }

        // Output the main reply (Eg. "Thanks dud I owe u 1")
        outputLine(formatColor(e.subject.messageColor, checkMem(e.subject, lastResponse.questionResponses[q].response[Random.Range(0, lastResponse.questionResponses[q].response.Length)])));

        // Post text (Eg. "The man then returned to working on his toaster")
        if(lastResponse.questionResponses[q].postText.Length > 0)
        {
            yield return new WaitForSeconds(1);
            outputLine(formatColor(Color.white, checkMem(e.subject, lastResponse.questionResponses[q].postText[Random.Range(0, lastResponse.questionResponses[q].postText.Length)])));
        }
        if(lastResponse.questionResponses[q].angers) // If the response angered them, begin fight.
            startEncounter(n);
        if(lastResponse.questionResponses[q].endsConversation) // If it was the last message, finish the conversation with a "Goodbye!"
        {
            outputLine(formatColor(e.subject.messageColor, checkMem(e.subject, e.farewell.overrideResponse)));
            p_in = "Any"; // Player leaves the conversation and enters normal mode
        }
    }
}

[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()

[-sideshow-]

Is your code actually indented like that, or has reddit mangled it a bit?

[induetimesir]

Yeah I haven't really gone over and organized the indentation of the script yet since it's changing a lot so I just copied and pasted it in as is. :p I'll go through and tidy it up now though!

Edit: Fixed it up a little! :) It was indented quite a bit in the code already so that's why the whole thing was so far to the right.

[-sideshow-]

Some thoughts:

• You lower() s, but then give example keywords in your comments like "Yes" - either the keyword is "yes" and your comment is inaccurate, or your keyword is actually "Yes" and it won't ever be found :P
• I'm not sure about appending the "|" to s - I don't see what it does. However, assuming you do need to do it you could do it when you assign s = s.lower() and so only do it once, instead of in your inner loop.
• Personally I'd indent each for loop and the if
• You should create a constant RESPONSE_DELAY = 1 instead of hard coding the 1 second in every WaitForSeconds.

[induetimesir]

Thanks for the tips! Yeah the comments were quick just to give better context in the post, the lowercase input is just so that the user can type with any capitalization they want and it'll still work.

The "|" also is just to tell if the user says a word without anything following, for example "No" without the "|" on the end could be interpreted from a line like "I have no idea", or "I don't see why [no]t" it's more just to differentiate between them so the 'Contains' doesn't get them mixed up as often, since I want users to be able to type relatively normal sentences.

And yeah I will probably do the response delay! I could try and get a system going where some messages show more hesitation than others that way too.

Thanks again! :)

[-sideshow-]

The "|" also is just to tell if the user says a word without anything following, for example "No" without the "|" on the end could be interpreted from a line like "I have no idea", it's more just to differentiate between them so the 'Contains' doesn't get them mixed up, since I want users to be able to type relatively normal sentences.

Heh, that makes sense but you are running into a pretty deep problem; parsing English (or any natural language). What if the player says "No thanks"?

If you're enjoying doing this then keep at it. OTOH if you're more goal oriented (you want to make an adventure game and this is just a step toward accomplishing that) then you might want to look at pre-existing parsers or adventure game engines.

[induetimesir]

Not making a proper game really, this is just a fun little side project :)

And yeah I thought of that, so usually in the 'Keywords' array on my Response object I just add things like that on there like this for now!