3
\$\begingroup\$

I have a pretty simple C# windows forms application working here. It is used for creating and running a Cubscout Pinewood Derby. You can create dens, assign them cars, generate a race schedule and then run races using an electronic timer and then view the race results.

I've ended up with sort of a class that basically is maintaining state of the application and have used databinding with a set of lists contained in it, to link various list boxes on the windows form. I've been able to write unit tests this way and am making steady progress.

Is there a more appropriate approach? This seems to take care of decoupling state from the Form/Main. I'm trying to up my design/testing/quality skills.

Here is some sample code of the class in question:

using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace esp8266_derby_app
{
    public class Derby
    {
        public Pack pack { get; set; } = new Pack();
        public List<Den> dens { get; set; } = new List<Den>();
        public List<Car> cars { get; set; } = new List<Car>();
        public List<Race> races { get; set; } = new List<Race>();
        public List<FinishTime> finishTimes { get; set; } = new List<FinishTime>();
        public List<List<Guid>> laneSchedule = new List<List<Guid>>();
        public List<Car> participants = new List<Car>();
        public bool saved { get; set; } = false;
        public string savedFileName { get; set; } = "";
        public string timerIP { get; set; } = "";
        public bool useTimer { get; set; } = false;
        public int heatsPerCar { get; set; } = 4;
        public int trackLanes { get; set; } = 4;
        public int redoRaceNumber = 0;

        public Guid AddDen(string name, string rank)
        {
            Den newDen = new Den();
            newDen.rank = rank;
            newDen.name = name;

            // remove den if it already exists (in case of editing)
            int idx = dens.FindIndex(a => a.Equals(newDen));
            if (idx != -1)
                dens.RemoveAt(idx);

            dens.Add(newDen);

            return newDen.ID;
        }

        public void DeleteDen(Guid denID)
        {
            // delete cars that belong to the den
            cars.RemoveAll(g => g.denID == denID);            

            int idx = dens.FindIndex(a => a.ID == denID);
            dens.RemoveAt(idx);
        }
    }
}

Sample code of an associated class:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace esp8266_derby_app
{
    public class Den
    {        
        public string rank { get; set; } = "";

        public string name { get; set; } = "";

        public List<Guid> carIDs { get; set; } = new List<Guid>();

        public Guid ID { get; set; } = Guid.NewGuid();

        public string DisplayMember { get { return rank + " - " + name; } }

        public override int GetHashCode()
        {
            return base.GetHashCode();
        }

        public override bool Equals(object obj)
        {
            Den that = obj as Den;
            if (this.name == that.name && this.rank == that.rank) 
                return true;
            else
                return false;
        }
    }
}

Sample unit test:

using NUnit.Framework;
using esp8266_derby_app;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace esp8266_derby_app.Tests
{
    [TestFixture()]
    public class DerbyTests
    {
        [Test()]
        public void AddDenTest()
        {
            Den den = new Den();
            den.name = "terrible tigers";
            den.rank = "tiger";

            Derby derby = new Derby();
            derby.AddDen(den.name, den.rank);

            Assert.Contains(den, derby.dens);
        }

        [Test()]
        public void AddDenTestAlreadyExists()
        {
            Den den = new Den();
            den.name = "terrible tigers";
            den.rank = "tiger";

            Derby derby = new Derby();
            derby.AddDen(den.name, den.rank);
            derby.AddDen(den.name, den.rank);

            Assert.Contains(den, derby.dens);
            Assert.AreEqual(derby.dens.Count, 1);
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Here is some sample code of the class in question - just so you know, don't expect anyone to review your repository. Most, if not all answers will review only the part you've posted here. The title is still misleading because there is no winforms code in your question. \$\endgroup\$
    – t3chb0t
    Commented Feb 16, 2017 at 13:17
  • \$\begingroup\$ Thanks for the insights, I probably rushed this post a bit. Will do better next time ;) \$\endgroup\$ Commented Feb 16, 2017 at 15:21

2 Answers 2

5
\$\begingroup\$

Style

When one is concerned with how the code is functioning, review points about style/conventions can seem nitpicky and irrelevant. The point is that although a compiler will accept anything that is syntactically correct, source code is also a communication to current and future maintainers/developers. The more familiar they find it, the faster and easier they will be able to understand it.

The standard C# coding conventions can be found at C# Coding Conventions.

Naming
In C#, public ids are Pascal Cased

e.g.

public class Derby
{
    public Pack Pack { get; set; } = new Pack();
    public ISet<Den> Dens { get; set; } = new Set<Den>();
    public List<Car> Cars { get; set; } = new List<Car>();
    public List<Race> Races { get; set; } = new List<Race>();
    //...

Approach

Decoupling the UI and the functionality and there are, presumably a lot of good guides out there on how to achieve it with WinForms. (It has been so long since I worked in WinForms that I am not qualified to comment on this area). However, looking at the code there a number of other design ideas that might be introduced.

Encapsulation/Data Hiding
All the setters are public. This means that any other code in the application can make changes to them.

Lets take the Cars list as an example. We create the Derby instance and when doing so create a list of Cars in it. However, because it is public, any other code can assign a different list and or null it. It is this intent?

Unless one wants the member to be externally writable, the setter should be made private

//...
public List<Car> Cars { get; private set; } = new List<Car>();
//...

There is a public member variable redoRaceNumber, is there a reason why this is a member variable and not a property? In general, public member variables are frowned upon.

Object initialization
Where possible, it is a good idea to have newly created object be in a valid state.

Take the Den class. By the looks of it, a Den should have a Name, a Rank, a Guid (we'll come back to that) and should be able to contain Cars.

Is there any situation where we would create a Den without a name, or a rank?

If not, we should try to create valid instances in one go. Can the name for a Den be changed? If not, then it should be readonly. Presumably, the rank can (based on win/loss record).

Are there a fixed set of ranks? If so, an enum is probably the way to go

public enum Rank
{
    None,
    Harmless,
    MostlyHarmless,
    Poor,
    Average,
    AboveAverage,
    Competent,
    Dangerous,
    Deadly,
    Elite
}

public Class Den
{
    private readonly IList<Car> _cars;

    public Den(string name, Rank rank = Rank.Harmless)
    {
         Name = name;
         Rank = Rank;
         _cars = new List<Car>();
    }

    public string Name {get;}

    public Rank Rank {get; set;}

    public IEnumerable<Car> Cars => _cars;

    public void AddCar(Car car)
    {
        _cars.Add(car);
    }

    public void RemoveCar(Car car)
    {
        _cars.Remove(car);
    }

    public override string ToString()
    { 
        return $"{Rank} - {Name}";
    }
}

Notes:
If the rank cannot be written as a string with no spaces (we'd like it to show as Above Average not AboveAverage then we can use the Description Attribute or a helper function to convert the rank to a string. Using the enum for limited sets instead of strings can reduce errors caused by mis-typing the names.

Is the Guid required for the Databinding to WinForms? If it is, we can reinstate them (the Id for the Den and the list of Cars) but if not, it is a lot simpler to work with lists of the actual objects instead of trying to look them up using Guids. Removing Den from a Derby now becomes

public RemoveDen(Den den)
{
    Dens.Remove(den);
}

Note:The name change is important in terms of communication - we are not deleting the Den, it may exist in other Derby instances (and nothing we have done in the code explicitly deletes it - though it may get Garbage Collected if nothing else is holding on to it) - we are simply removing it from the Derby.

By changing Cars to a private list with public methods for adding/removing cars we reduce coupling between he code using the Den and the implementation of the Den. The client code doesn't need to know that the Cars are stored in a list called Cars. This is not so important here but is for the Derby (see below). We also allow ourselves the opportunity to perform checks and/or trigger actions on adding/removing.

By changing the public member from a List to an IEnumerable we prevent client code from doing things like

//...
den.Cars.Clear();
//...

If the cars are to be removed from the Den, it should be the one doing so. If we need this functionality, we add a member to the Den. This aids our communication, now someone looking at the Den code knows that clearing all the Cars in the Den is a valid thing to do.

The Derby has a list of Cars, which (from the DeleteDen()) seems to be a list of all the Cars in each Den, or a list of the whichever of the Cars from a Den are entered in the Derby.

By implementing it as two lists we put a lot of synchronization work upon ourselves. Another way to do this is

  private class Derby
  {
     private readonly IDictionary<Den, IList<Car>> _entrants;

     public Derby()
     {
        _entrants = new Dictionary<Den, IList<Car>>();
     }

     public IEnumerable<Den> Dens => _entrants.Keys;

     public IEnumerable<Car> Cars => _entrants.SelectMany(e => e.Value);

     public void AddEntrant(Den den, Car car)
     {
        IList<Car> entrants;
        if (!_entrants.TryGetValue(den, out entrants)){
           entrants = new List<Car>();
           _entrants.Add(den, entrants);
        }
        entrants.Add(car);
     }

     public void RemoveDen(Den den)
     {
         _entrants.Remove(den);
     }

  }

We have limited the access points to the Derby. We can now add a race entrant and this will give us the correct lists of Dens and Cars. The internal workings of the Derby are not visible and can be changed without impacting client code. The Derby is no longer responsible for creating dens - it shouldn't be, it's a competition - but if we need/want to make it do so we can with

//...
public Den AddDen(string name, Rank rank)
{
    _entrants.Add(new Den(name, rank), new List<Car>());
}
//...

I have left out the replacing functionality because it seems to be a design problem. Under which use case will we try to add a Den a second time and want to replace any existing Den with the new one. Management of the Dens should be elsewhere.

Overriding Equals
There are sets of recommendations for overriding the equals operator

Usage Guidelines - Equality Operators
Implementing Equals
Guidelines for implementing Equals() and ==

The second and third have details of what to do - note: there is little use in overriding the GetHashCode() function and calling the base function. The intent of GetHashCode() is that two instances that we want to be considered as equal must return the same hash value. This will not happen if we do not implement a GetHashCode() based upon the how the application values the object.

The first is more useful here. We only need to implement them if we have multiple instances of an object which will have the same values and which we want to consider to be the same object. Will this happen in the this application?

\$\endgroup\$
2
  • \$\begingroup\$ All very good points - I guess my key question is with databinding, don't I have to make the various Lists public, so that they can be bound? (i.e. allowing the forms elements to access them through the binding source?) \$\endgroup\$ Commented Feb 16, 2017 at 15:24
  • \$\begingroup\$ As I said, It's been a while since I did any work with WinForms so it might be necessary (In WPF, I work mainly with IEnumerables and have viewmodels sitting between the screen and the app models) Anything that you need for databinding keep doing it - but... say you want to turn this into a web app (or a WPF app) which approaches databinding differently. The core design of the app should represent the way we want the application to work not the needs of a specific UI. There may be other options for making the databinding work that do no require public lists. \$\endgroup\$
    – AlanT
    Commented Feb 16, 2017 at 15:43
-1
\$\begingroup\$

AlanT's review is great so this amended post will address only the items I feel have been missed, which are mostly business modelling. I've added a gist here with an updated set of interfaces, and some partial class implementations.

Data Modelling

Modelling is always a fun topic, and inaccurate modelling of the business objects is often a cause for many a headache. In this case, I think there's some improvements that can be made on the model.

I've made the following assumptions:

  • A Den is a group of n cub scouts who can each race one car in the Derby.
  • A Pack is a group of n Dens.
  • A Derby is an event where n cub scouts from n dens attend and race their cars. It has n Races.
  • A Race is composed of n of Cars, each of which are assigned a finish time once the race is finished. Each car is assigned to a lane in the race.
  • For the purpose of this analysis, we can conflate Car and the Cub Scout into one entity.

Implicitly, the existing relationships then also exist:

  • A Derby has n car entrants.
  • A Car can be entered into n Derbys, and has participated in n Races.

Your existing model then has some flaws:

Race Participation

FinishTime exists under the Derby as a separate list, but it is not directly related to the Derby. Instead, it should exist solely as a property of the Race it has been entered under. It should be accessible as a Get Only property The FinishTime is also inappropriately named because it does more than just capture the FinishTime. It is a representation of the car's participation in the race, and therefore should be named a RaceParticipant. This way, it is clearler what the object actually models.

public interface IRaceParticipant
{
    IRace Race { get; }
    IDerbyParticipant DerbyParticipant { get; }
    TimeSpan Result { get; set; }
    int Lane { get; }
    /// <summary>
    /// Provides name from <see cref="IDerbyParticipant"/>.
    /// </summary>
    string ScoutName { get; }
}

Derby Participation

If this application will be used for more than one Derby, then you have the possibility that not all Cars in each Den will be raced. Also, you will want to record the winner, rankings, and other information as necessary. Consequently, you will want a DerbyParticipant class that represents the relationship of a Car participating in the Derby, and in multiple Races.

public interface IDerbyParticipant
{
    IRace Race { get; }
    ICar Car { get; }
    /// <summary>
    /// Makes the scout name accessible, but redirects to
    /// <see cref="ICar.ScoutName"/>.
    /// </summary>
    string ScoutName { get; }
    int FinalRank { get; set; }
}

Other Notes

Knowledge of Neighbours

Your code should endeavour to adhere to the Law of Demeter. This means only referring to the closest neighbour. Here's some examples:

class RaceParticipant : IRaceParticipant {
    ...
    public string ScoutName => DerbyParticipant.Car.ScoutName; // Bad   
    public string ScoutName => DerbyParticipant.ScoutName; // Good
    ...
}

class Derby : IDerbyParticipant {
    ...
    public IRaceParticipant BestParticipant => Races.SelectMany(x => x.Participants)
        .OrderByDescending(y => y.Result).First(); // Bad

    public IRaceParticipant BestAltParticipant => Races.Select(x => x.BestParticipant).
        OrderByDescending(y => y.Result).First(); // Good
}

These examples are small violations, but when you work with larger code bases you run into big problems if you have code that is dependent on another class's methods on another class's methods like so:

class ZhuLi {
    void DoTheThing() {
        Things.First().Thing.OtherThing().Thing().OtherThing();
    }
}

In this case, changing any one of those interfaces would require this line of code to be changed. The multiply for each time that method/property on the interface has been references. It gets real ugly real fast.

Use an ORM

I've found that using an ORM is generally helpful because it allows you to abstract the mechanics of data persistence (Managing object IDs and list of related IDs) outside of your code and instead allows you to deal with the business objects. They may not scale well but I think something like SQLite.Net or Realm will help here.

Test with Mocks

You'll want to use a mocking library (I prefer Moq myself) for any neighbours. This way, you can directly test when a method was called, what arguments it received and so on. Using Interfaces makes using a library like Moq easier as Moq only allows you to work with virtual methods and properties. This is why you...

Always Use Interfaces

Whenever you're dealing with any reference type you should be referring to an interface unless you're instantiating the object. This allows you to decouple implementation from the code contract. It also allows you to easily insert mock values, whereas specifying a class makes that more challenging.

interface IThing {
    void DoThing();
}

class Thing : Thing {
    Thing OtherThing(); // Must always return a Thing. Things that behaves like a IThing cannot be returned.
    IThing Thing(); // Can return anything that behaves like a IThing, even if it's not a Thing.
}

Always Use TimeSpan for Durations

Always use TimeSpan values to represent a duration. Using numeric values type (int, float, decimal) is not recommended because they are scalar values only and do not include the time unit. Otherwise you could run into conversion issues similar to those that caused the Mars Climate Orbiter crash.

class Result {

    decimal ResultTime {get; set;} // So is this in seconds? Minutes? Milliseconds?
    decimal ResultTimeInMilliseconds {get; set;} // Better, but it doesn't stop "ResultTimeInMilliseconds = TimeInSeconds".
    TimeSpan ResultTime {get; set;} // It carries the unit of time, and enforces math operators to do the same.
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I really like the actual remarks you made. i don't like that the way you present them makes it pretty hard to identify them in the first place and additionally that they can't be explained fully. That's why I downvoted this post ... \$\endgroup\$
    – Vogel612
    Commented Feb 16, 2017 at 10:50
  • \$\begingroup\$ @Vogel612 thanks for the feedback! I'll revise accordingly. :) \$\endgroup\$ Commented Feb 16, 2017 at 18:29
  • \$\begingroup\$ @Vogel612 revised! \$\endgroup\$ Commented Feb 16, 2017 at 20:30

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.