4
\$\begingroup\$

After the tips from my previous review. I have come up with these changes:

  • Separation of Concerns
  • Use Delegates

I'm asking you guys if this looks good. Other then separate out the FileButton_Click(), WriteResultsToFile(), and PromtSaveAsFileName() functions to another class would you recommend any other things to improve?

public partial class StopWatchForm : Form
    {
    WatchState state;
    Stopwatch stopWatch;
    TimeSpan timeSpan;
    List<TimeSpan> times = new List<TimeSpan>();

    public event Action OnStart = () => { };
    public event Action OnStop = () => { };
    public event Action OnLap = () => { };

    public StopWatchForm()
    {
        InitializeComponent();
        state = WatchState.Stopped;
        stopWatch = Stopwatch.StartNew();
        stopWatch.Stop();
        ConfigureStopWatchBehavior();
        ConfigureTimerBehavior();
        ConfigureTimesBehavior();
        ConfigureViewBehavior();
        ConfigureViewStateBehavior();
    }

    private void StartAndLapButton_Click(object sender, EventArgs e)
    {
        if (state == WatchState.Stopped)
        {
            OnStart();
        } else if (state == WatchState.Started)
        {
            OnLap();
        }
    }

    private void StopButton_Click(object sender, EventArgs e)
    {
        if (state == WatchState.Started)
        {
            OnStop();
        }
    }

    private void FileButton_Click(object sender, EventArgs e)
    {
        if (!times.Any())
        {
            return;
        }
        var filename = PromtSaveAsFileName();
        if (filename == null)
        {
            return;
        }
        WriteResultsToFile(filename);
        times.Clear();
    }


    private void ConfigureStopWatchBehavior()
    {
        OnStart += stopWatch.Restart;
        OnStop += stopWatch.Stop;
    }

    void ConfigureTimesBehavior()
    {
        Action storeElapsedtime = () => times.Add(stopWatch.Elapsed);
        OnLap += storeElapsedtime;
        OnStop += storeElapsedtime;
    }

    void ConfigureViewBehavior()
    {
        OnStart += () =>
        {
            StartAndLapButton.Text = "Lap";
            this.Text = "Started";
        };

        OnStop += () =>
        {
            StartAndLapButton.Text = "Start";
            this.Text = "Stop Watch";
        };
    }

    void ConfigureViewStateBehavior()
    {
        OnStart += () => state = WatchState.Started;
        OnStop += () => state = WatchState.Stopped;
    }

    void ConfigureTimerBehavior()
    {
        OnStart += timer1.Start;
        OnStop += timer1.Stop;
    }

    private void WriteResultsToFile(string filename)
    {
        List<string> timesToString = new List<string>();
        foreach (var item in times)
        {
            TimeSpan newitem = new TimeSpan(item.Hours, item.Minutes, item.Seconds);
            timesToString.Add(newitem.ToString());
        }
        System.IO.File.WriteAllLines(filename, timesToString);
    }

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

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

        return null;
    }

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

This is the 3rd iteration of this code. If you would like to see the original post here is the link.

\$\endgroup\$
4
  • \$\begingroup\$ Wow. This barely looks like the same code! \$\endgroup\$
    – RubberDuck
    Nov 18, 2014 at 19:19
  • \$\begingroup\$ Is that a good barely or bad barely? My skills as a programmer aren't great. I just want to get good enough to get a junior position so I can be around programming all day. \$\endgroup\$
    – Funlamb
    Nov 18, 2014 at 19:26
  • \$\begingroup\$ It's a good wow. Looks like you've learned a lot. I haven't seen it since you're original question. I haven't really looked at the newest version yet beyond an initial impression of great improvement over the first post. By the way, have you been to our chat room yet? \$\endgroup\$
    – RubberDuck
    Nov 18, 2014 at 19:33
  • \$\begingroup\$ The chat room! Wow thanks for that and your words of encouragement. \$\endgroup\$
    – Funlamb
    Nov 18, 2014 at 19:37

1 Answer 1

3
\$\begingroup\$
stopWatch = Stopwatch.StartNew();
stopWatch.Stop();

If you want to create a Stopwatch that's stopped, use the constructor instead of StartNew() and then immediately stopping it.


timer1

The autogenerated WinForms names don't make much sense. If you have only one timer and there is no need to name it, call it just timer.

Also, consider creating the Timer in code, so that I don't have to look at the designer to find out for example what the period of timer is (or to change it).


if (state == WatchState.Stopped)
{
    OnStart();
} else if (state == WatchState.Started)
{
    OnLap();
}

This is an unusual bracing style. The normal .Net style would be to have the else if on a new line.


TimeSpan newitem = new TimeSpan(item.Hours, item.Minutes, item.Seconds);
timesToString.Add(newitem.ToString());

What is the purpose of this code? Is it to get rid of the "milliseconds" part in the output? Are you aware that it also removes the "days" part? If you're doing this just to get the format you want, consider using a custom format string (.Net 4+ only).


string elapsedTime;

There is no reason to declare this variable early.


String.Format("{0:00}:{1:00}:{2:00}", timeSpan.Hours, timeSpan.Minutes, timeSpan.Seconds)

Why are you using two different approaches to get TimeSpans formatted in the same way (here and using the new TimeSpan() approach above)? Considering encapsulating the formatting into a common method.

\$\endgroup\$
1
  • \$\begingroup\$ I changed the stopWatch to use the constructor like you said. I just couldn't figure out how to do it and all the questions on here showed to just add it with Stopwatch.StartNew(). I was not aware of the custom format string which you speak of. Thanks for the link it was a huge help. The only advise I didn't use was creating a timer in code. I should do it but it's 10:00 and I'm just so tired. Maybe in the morning. Thanks for your input. \$\endgroup\$
    – Funlamb
    Nov 19, 2014 at 5:45

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.