2
\$\begingroup\$

I have windows form application. One button event starts the script via object called ScriptStarter. The status of object is used to pass message and color to UI (toolStripStatusLabel1). I'm not sure if this is good practice. Could you please give me advice or suggestion how to better set up the communication between object and UI? I have heard little about Action(). I don't know if should I consider using of this. Any suggestions are warmly welcomed.

    private void btnStartScript_Click(object sender, EventArgs e)
    {
        ScriptStarter scriptStarter = new ScriptStarter();
        scriptStarter.StartScript();
        toolStripStatusLabel1.Text = scriptStarter.Status.Message;
        toolStripStatusLabel1.ForeColor = scriptStarter.Status.FontColor;
    }

The class ScripStarter.cs is here (I've replaced some methods body with comment in this post):

class ScriptStarter
    {
        public StatusKeeper Status = new StatusKeeper();

        public void StartScript()
        {
            string scriptFilePath = Properties.Settings.Default.MyStartCacheScriptPath;
            if (scriptFilePath.Length == 0 || !File.Exists(scriptFilePath))
                scriptFilePath = setNewScriptFile();

            if (File.Exists(scriptFilePath))
            {
                tryToRunScript(scriptFilePath);
            }
            else
            {
                Status.Message = "Script not found";
                Status.FontColor = Color.Crimson;
            }
        }

        private void tryToRunScript(string scriptFilePath)
        {
            bool succeeded = runBatScript(scriptFilePath);
            if (succeeded)
            {
                Status.Message = "Script started";
                Status.FontColor = Color.ForestGreen;
            }
            else
            {
                Status.Message = "Script not started";
                Status.FontColor = Color.Crimson;
            }
        }

        private bool runBatScript(string scriptFilePath)
        {
            // returns true if run script was successful
        }

        private string setNewScriptFile()
        {
            // returns script path
        }
    }

The class StatusKeeper.cs

public class StatusKeeper
{
    public string Message { get; set; }
    public Color FontColor { get; set; }
}
\$\endgroup\$
2
\$\begingroup\$
public StatusKeeper Status = new StatusKeeper();

It's a very bad idea to return the result via a public field.

If anything bad happens you should throw an exception. By doing this you automatically will need to move the stylings to somewhere else but this is what we want - separate different responsibilities. Your class is doing too much right now. The ScriptStarter shouldn't know anything about the colors etc. Its only responsibility should be to start scripts. It could be much simpler like this:

class ScriptStarter
{   
    public void RunScript()
    {
        var scriptFilePath = Properties.Settings.Default.MyStartCacheScriptPath;
        if (string.IsNullOrEmpty(scriptFilePath) || !File.Exists(scriptFilePath))
        {
            scriptFilePath = setNewScriptFile();
        }

        if (!File.Exists(scriptFilePath))
        {
            throw new FileNotFoundException("Script not found.", scriptFilePath);
        }

        runBatScript(scriptFilePath);
    }   

    private void runBatScript(string scriptFilePath)
    {
        // do something and throw if it didn't work
        throw new ScriptStartException(); // create a new type of exception for this
    }

    private string setNewScriptFile()
    {
        // returns script path
    }
}

How do we run this now?

We write a new method that will use the ScriptStarter and handle the results:

private void RunScript() 
{
    try
    {
        ScriptStarter.RunScript();
        UpdateScriptStatusLabel("Script started", Color.ForestGreen);
    }
    catch (FileNotFoundException)
    {
        UpdateScriptStatusLabel("Script not found.", Color.Crimson);
    }
    catch (ScriptStartException)
    {
        UpdateScriptStatusLabel("Script not started.", Color.Crimson);
    }
    catch (Exception ex)
    {
        UpdateScriptStatusLabel($"Something else happened: {ex.Message}", Color.Red);
    }
}

We also need another one that will take care of the status label:

private void UpdateScriptStatusLabel(string text, Color color) 
{
    toolStripStatusLabel1.Text = text;
    toolStripStatusLabel1.ForeColor = color;
}

Now that we have separated all concerns we can finally update the event handler:

private void btnStartScript_Click(object sender, EventArgs e)
{
    RunScript();
}

What did we win? We won maintainability and reusability. If you decide to add a keyboard shortcut in a future version then you just call the RunScript() from there and that's it. No repetition of the formatting or the logic how to run the script.

\$\endgroup\$
  • 1
    \$\begingroup\$ You also gained the ability to run the script asynchronously, too :-) \$\endgroup\$ – Mathieu Guindon Oct 30 '16 at 15:00
  • \$\begingroup\$ Hi @Mat'sMug, many thanks for your code review. I'm going to implement your thoughts. I know I need to practice more SOLID principles. \$\endgroup\$ – Tomáš Paul Oct 30 '16 at 20:55
  • \$\begingroup\$ @TomášPaul the review was a courtesy of t3chb0t, but no problem =) \$\endgroup\$ – Mathieu Guindon Oct 30 '16 at 20:56
  • 1
    \$\begingroup\$ Hi @t3chb0t, thank you for code review and your advice! \$\endgroup\$ – Tomáš Paul Oct 30 '16 at 21:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.