Windows forms application for a Cubscout Pinewood Derby

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 dens { get; set; } = new List();
public List cars { get; set; } = new List();
public List races { get; set; } = new List();
public List finishTimes { get; set; } = new List();
public List> laneSchedule = new List>();
public List participants = new List();
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)

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 Dens { get; set; } = new Set();
    public List Cars { get; set; } = new List();
    public List Races { get; set; } = new List();
    //...

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 Cars { get; private set; } = new List();
//...

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

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

    public string Name {get;}

    public Rank Rank {get; set;}

    public IEnumerable 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 t

StackExchange Code Review Q#155486, answer score: 5