3
\$\begingroup\$

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
    ///
}
}
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

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.
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your input. I need to just give up my Java roots and stick with language conventions when they are there. I'd like to post the new code but I feel it would bloat my post even further. Is there a convention codereview has to do that? \$\endgroup\$
    – Funlamb
    Commented Nov 11, 2014 at 17:34
  • \$\begingroup\$ Sure! See this meta post for all the details about iterative reviews (please don't edit your question) :) \$\endgroup\$ Commented Nov 11, 2014 at 17:36
4
\$\begingroup\$
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.

\$\endgroup\$
4
  • \$\begingroup\$ I see your point on the namespace. I just named my folder [Hard0002]StopWatch so I can keep track of the challenges. Is there a way better way to name my folders to keep them organized and not have the weird underscore? \$\endgroup\$
    – Funlamb
    Commented Nov 11, 2014 at 1:13
  • \$\begingroup\$ Well, I created a Challenges namespace and then nested a namespace for each different site and then each prompt got its own file. Seemed to work all right for me. \$\endgroup\$
    – RubberDuck
    Commented Nov 11, 2014 at 1:34
  • \$\begingroup\$ If that style is Egyptian, what is the other style? \$\endgroup\$
    – ADTC
    Commented Nov 11, 2014 at 12:39
  • 2
    \$\begingroup\$ @ADTC "Egytian" is really a variant of the K&R style, while the "C#" style is actually the Allman style. See the style section on this Wikipedia article. \$\endgroup\$
    – RubberDuck
    Commented Nov 11, 2014 at 12:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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