8
\$\begingroup\$

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

  1. Changed my braces to start on a new line
  2. Used the Single Responsibility Principle
  3. Return early whenever possible
  4. Use a namespace that doesn't start with an underscore

I'm not entirely sure I have used the namespace correctly. If it's wrong, can you please give me a push in the right direction? Any other comments would be appreciated.

namespace Difficult.StopWatch0002
{
    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");
            ChangeFormTitle("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");
            ChangeFormTitle("Stop Watch");
            stopWatch.Stop();
            times.Add(elapsedTime);
            state = StateEnum.Stoped;
            timer1.Enabled = false;
        }
    }

    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 WriteResultsToFile(string filename)
    {
        System.IO.File.WriteAllLines(filename, times);
    }

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


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

    private void ChangeFormTitle(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;
    }
}
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't have a full answer, but Microsoft says namespaces should be approximately: CompanyName.TechnologyName[.Feature][.Design] - so perhaps MikesPracticeApps.Stopwatch. \$\endgroup\$ – Magus Nov 11 '14 at 20:15
3
\$\begingroup\$

Use Correct Types in the Model

This is your model:

Stopwatch stopWatch;
TimeSpan timeSpan;
string elapsedTime;
List<string> times = new List<string>();

This is an example of stringly typed programming. Store data in meaningful types and convert them to other types (string, byte[] etc) for transfer, display etc. as necessary. For example storing times as List<TimeSpan> you can convert this to a list of running totals or display a graph etc without parsing, or worse trying to parse, the strings back to a meaningful type.

Looking from another perspective, you model should depend on the requirements, and as little else as possible. reading the problem description, and not bothering with menial details, thus not putting the cart before the horse, you would come up with some model like this:

Stopwatch stopWatch; // from (start lap stop) requirement 
List<TimeSpan> times = new List<TimeSpan>(); // from (write to file) requirement

Separation of Concerns

We see here several consequences of a user action bunched up:

stopWatch = Stopwatch.StartNew();
state = StateEnum.Started;
ChangeButtonText(StartAndLapButton, "Lap");
ChangeFormTitle("Started");
timer1.Enabled = true;

Suppose you decided to change one aspect, say how times are stored, then you will have to look for every code block that refers to it, and try to judge whether the change you will make affect the behavior of the code in the same code block below the changes.

Another way of organizing things is the OO way, group data and related behavior together:

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

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 = StateEnum.Started;
    OnStop += () => state = StateEnum.Stoped;
}

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

Which causes form event handlers to look like these:

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

private void StopButton_Click(object sender, EventArgs e)
{
    if (state == StateEnum.Started) OnStop();
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the thorough answer. I have one question. What is this, event Action OnStart = () => {};? I've never seen it before and don't know how to look it up. \$\endgroup\$ – Funlamb Nov 12 '14 at 18:21
  • \$\begingroup\$ event Action OnStart is an event declaration. And () => {} is just an Action that does nothing. Events are like collections of delegates, initializing an event to () => {} is like initializing an empty collection. It allows us, in this case, to avoid if (OnStart != null) checks everywhere we invoke OnStart. \$\endgroup\$ – abuzittin gillifirca Nov 13 '14 at 8:08
  • \$\begingroup\$ Event declaration went way over my head. After reading through the event tutorial and delegates tutorial it looks like this is what's used for the Observer Design Pattern. Am I right in that? \$\endgroup\$ – Funlamb Nov 13 '14 at 18:33
  • \$\begingroup\$ After working on my code some more. I've realized I never was able to separate my concerns. All I've done is ctrl R, m and moved it over to another method. I'll be looking over event declaration and delegates before I post my code. Is there a different tutorial to learn these concepts? Is this really important to learn at my skill level right now or should I just do some more simple project? \$\endgroup\$ – Funlamb Nov 13 '14 at 19:39
  • \$\begingroup\$ Delegates are a prerequisite not only for events but also for LINQ. And they are everywhere, that you have an understanding of them and a familiarity with their syntax is important to be able to read C# code. \$\endgroup\$ – abuzittin gillifirca Nov 14 '14 at 7:14

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.