Stop Watch application

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 times = new List();

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);

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.

StackExchange Code Review Q#69435, answer score: 6