Dialogue system using an FSM

In short - I've started with C# not so long ago (which means you find a lot of poor written code here) and now I'm aiming to create a dialogue system. Though my code is working as it supposed to, questions are:

• How to make everything not this lame and improve the code

• Suggestions about improving performance

• Overall advice (maybe there is some more suitable tools for doing this)

Right now I use Finite-State Machine (FSM) as a general concept, so that every state is a dialogue scene. The last one is made of NPC quote and set of Player responds. By now everything is pretty basic.

Here I have my class for Player responds:

using System;
using System.Drawing;
using System.Windows.Forms;
using System.IO;

namespace Testing
{
    public class Player_Qoute : Label
    {
        public string Next_State { get; set; }//is used to tell, where to go next after choosing particular respond
    }

It's inherited from Label and has one additional field - next stage number, which is added in this function:

private void NPC_Quote(string path, string specification, RichTextBox info)
{
    StreamReader file = new StreamReader(path);//creating StremReader to read NPC quotes from .txt
    string line = "";//Creating string variable to read from
    try
    {
        while ((line = file.ReadLine()) != null)//reading file line-by-line until the end
        {
            if (line.Contains(specification))//if line contains specified "tag"
            {
                line = line.Remove(0, specification.Length);//removing "tag" from line
                info.Text += line + "\n";//adding NPC line to the output field 
            }
        }
        file.Close();
    }
    catch (Exception)
    {
        MessageBox.Show("Problem reading file");
    }
}

This function parse through .txt file, searching for lines tagged NPC_stage_n, where n - is a number of stage. This number is present at the end of every Player respond in .txt file and I put it in the `Next_

I want to talk about just one aspect of what makes this particular method so difficult to read, understand and maintain:

private void NPC_Quote(string path, string specification, RichTextBox info)
{
    StreamReader file = new StreamReader(path);//creating StremReader to read NPC quotes from .txt
    string line = "";//Creating string variable to read from
    try
    {
        while ((line = file.ReadLine()) != null)//reading file line-by-line until the end
        {
            if (line.Contains(specification))//if line contains specified "tag"
            {
                line = line.Remove(0, specification.Length);//removing "tag" from line
                info.Text += line + "\n";//adding NPC line to the output field 
            }
        }
        file.Close();
    }
    catch (Exception)
    {
        MessageBox.Show("Problem reading file");
    }
}

Let's try to describe what this method does in English. It opens a file, reads every line of the file looking for a line with a particular tag, removes the tag, updates a text box, and handles errors.

Did you notice something about that description? It said nothing whatsoever about the business domain of the method! The "business domain" is a description of what the method does in context of what the program is for: interacting with an NPC in a game.

What you've done here is thoroughly mixed up back-end mechanism logic -- opening files, searching for a tag -- with display logic -- updating a text box -- with error management logic, and with business logic. Methods that try to do so much mean that it becomes impossible to change the rules of the game - the actual business of the program - without changing code that touches all the mechanisms that underly it.

Refactor all of this stuff. You have a concept: NPC. Make a type that represents the NPC.

class NPC
{
  private static string quotesFile = @"npcquotes.txt";
  static public string Quote(string tag)
  {
    var line = File.ReadLines(quotesFile)
      .Where(line => line.StartsWith(tag))
      .Select(line => line.Remove(0, tag.Length))
      .FirstOrDefault();
    return line ?? "";
  }
}

Good heavens look at how much nicer that is. (Not to mention the correctness errors that I've fixed. If your tag was at the end of a string it would still be matched in your implementation.)

Now you can say NPC.Quote(tag) and get back a string. Now write a class that represents the user interface of the game. That is the thing that should be updating the text box, not the code that reads the NPC strings.

Moreover: if later on you decide that you want to have the strings in memory instead of in a file, now you have only ONE place to change the code:

class NPC
{
  private static string quotesFile = @"npcquotes.txt";
  private Dictionary quotes = new Dictionary()
  {
     { "blah", "blah blah blah blah" },
     { "foo", "foo bar foo bar" }
  };
  static public string Quote(string tag)
  {
    return quotes.ContainsKey(tag) ? quotes[tag] : "";
  }
}

By encapsulating the logic in a method that does only one thing you become free to change the implementation of that method without impacting the UI, the game logic, and so on.

What about the error handling? Well, you need to think this through better anyways. What experience do you really want the user of the game to have when an error occurs? Is a dialog that says "file operation failed, you are borked" really that useful? Work out carefully what the error handling semantics are for your program and then implement them.

StackExchange Code Review Q#120884, answer score: 7