Stop Watch application

Question

I've done the difficult dailyprogrammer challenge #2:

Your mission is to create a stopwatch program. this program should have start, stop, and lap options, and it should write out to a file to be viewed later.

I wanted to see from you guys if what I did was correct. I'm looking for shortcuts or better coding practices I could have missed. This is a Windows Form Application.

namespace _Hard0002_StopWatch {
public partial class StopWatchForm : Form {
    StateEnum state;
    Stopwatch stopWatch;
    TimeSpan timeSpan;
    string elapsedTime;
    List<string> times = new List<string>();

    public StopWatchForm() {
        InitializeComponent();
        state = StateEnum.Stoped;
    }

    private void StartAndLapButton_Click(object sender, EventArgs e) {
        if (state == StateEnum.Stoped) {
            stopWatch = Stopwatch.StartNew();
            state = StateEnum.Started;
            ChangeButtonText(StartAndLapButton, "Lap");
            ChangeFormTittle("Started");
            timer1.Enabled = true;
        } else if (state == StateEnum.Started) {
            times.Add(elapsedTime);
        }
    }

    private void StopButton_Click(object sender, EventArgs e) {
        if (state == StateEnum.Started) {
            ChangeButtonText(StartAndLapButton, "Start");
            ChangeFormTittle("Stop Watch");
            stopWatch.Stop();
            times.Add(elapsedTime);
            state = StateEnum.Stoped;
            timer1.Enabled = false;
        }
    }

    private void FileButton_Click(object sender, EventArgs e) {
        saveFileDialog1 = new SaveFileDialog();
        saveFileDialog1.Filter = "txt files (*.txt)|*.txt|All files (*.*)|*.*";
        saveFileDialog1.FilterIndex = 2;
        saveFileDialog1.RestoreDirectory = true;

        if (times.Count > -1) {
            if (saveFileDialog1.ShowDialog() == DialogResult.OK) {
                string path = saveFileDialog1.FileName;
                System.IO.File.WriteAllLines(path + ".txt", times);
                times.Clear();
            }
        }
    }

    void ChangeButtonText(Button button, string text) {
        button.Text = text;
    }

    private void ChangeFormTittle(string text) {
        this.Text = text;
    }

    private void timer1_Tick(object sender, EventArgs e) {
        timeSpan = stopWatch.Elapsed;
        elapsedTime = String.Format("{0:00}:{1:00}:{2:00}", timeSpan.Hours, timeSpan.Minutes, timeSpan.Seconds);
        TimeKeeperText.Text = elapsedTime;
    }

    ///Your mission is to create a stopwatch program.
    ///this program should have start, stop, and lap options, 
    ///and it should write out to a file to be viewed later.
    ///

    ///Nouns:
    ///Start
    ///lap
    ///stop
    ///file (txt)
    ///
    ///Verbs:
    ///Keeps time
    ///Writes file
    ///
}
}

Answer 1

There is a lot to review here, so I'll just cover this handler:

private void FileButton_Click(object sender, EventArgs e) {
    saveFileDialog1 = new SaveFileDialog();
    saveFileDialog1.Filter = "txt files (*.txt)|*.txt|All files (*.*)|*.*";
    saveFileDialog1.FilterIndex = 2;
    saveFileDialog1.RestoreDirectory = true;

    if (times.Count > -1) {
        if (saveFileDialog1.ShowDialog() == DialogResult.OK) {
            string path = saveFileDialog1.FileName;
            System.IO.File.WriteAllLines(path + ".txt", times);
            times.Clear();
        }
    }
}

It's very easy to cram all of a functionality's logic inside an event handler in WinForms; it's a little bit harder to handle a Click event and not have the Single Responsibility Principle take a beating. This handler:

• Configures and displays a SaveFileDialog.
• Writes to the specified file.
• Clears the times list.

These are 3 distinct operations, and yet they all happen in the FileButton's Click handler.

One of my favorite refactorings, is the Extract Method functionality - in Visual Studio, there's a convenient keyboard shortcut for it: just select the code you want to extract, and then Ctrl+R, M.

I would select the first part, extract it into its own function, and get this:

private string PromptSaveAsFileName()
{
    var dialog = new SaveFileDialog
        {
            Filter = "txt files (*.txt)|*.txt|All files (*.*)|*.*",
            FilterIndex = 2,
            RestoreDirectory = true
        };

    if (dialog.ShowDialog() == DialogResult.OK)
    {
        return dialog.FileName;
    }

    return null;
}

This would leave the handler, with a few little more changes, like this:

private void FileButton_Click(object sender, EventArgs e) {

    if (times.Count == -1)
    {
        return;
    }

    var fileName = PromptSaveAsFileName();
    if (fileName == null)
    {
        return;
    }

    System.IO.File.WriteAllLines(fileName + ".txt", times);
    times.Clear();
}

It's still more than I'd like to see, but it's already much better. A few question marks before going any further:

• Under what circumstances would times.Count be -1?

  Never. The Count property of an empty list will return 0.

• Since we have a function whose role is to return our SaveAsFileName, who's responsibility is it to ensure the file has a .txt extension?

  The function should return the full file name, ready to be used, including its extension.

So. I'd do whatever I have to do, to make the handler look like this:

private void FileButton_Click(object sender, EventArgs e)
{
    if (!times.Any())
    {
        return;
    }

    var fileName = PromptSaveAsFileName();
    if (fileName == null)
    {
        return;
    }

    WriteResultsToFile(fileName);
    times.Clear();
}

Notice a couple more points:

• { scope opening brace is placed on the next line. This is C# convention; opening the scope on the same line is Java convention. I like that your brace style is consistent though, so that's just a minor point.
• Return early, and don't work for nothing: the above code will not set up a new SaveFileDialog and then return if there's nothing to save.
• Avoid extraneous nesting: it's often possible to reduce nesting by simply reversing a condition.

Answer 2

namespace _Hard0002_StopWatch {

It's nitpicky, but I've never seen a namespace begin with an underscore. It just looks weird to me.

While I'm being nit picky, C# tends to have braces on new lines instead of the Egyptian style that you use. But, you were consistent and that's what really matters.

You have a typo here.

private void ChangeFormTittle(string text) {
   this.Text = text;
}

But it doesn't matter much, because this method is kind of a useless abstraction. I would just directly set this.Text = "Started", etc.