Organizing class... best practices?

Question

Say I have a class as follows

    class FootballPlayer
    {
        public string Name { get; set; }
        public string TeamName { get; set; }

        public int CareerGoals { get; set; }
        public int CareerAssists { get; set; }
        public int CareerPasses { get; set; }

        public int SeasonGoals { get; set; }
        public int SeasonAssists { get; set; }
        public int SeasonPasses { get; set; }

        public int CurrentMatchGoals { get; set; }
        public int CurrentMatchAssists { get; set; }
        public int CurrentMatchPasses { get; set; }
    }

But I want to organize it better, so how can I do it. At the moment i tried something like this -

    class CareerDetails
    {
        public int Goals;
        public int Assists;
        public int Passes;
    }

    class CurrentMatchDetails
    {
        public int Goals;
        public int Assists;
        public int Passes;
        public bool IsCaptain;
    }

    class GeneralDetails
    {
        public string Name;
        public string TeamName;
    }

    class SeasonDetails
    {
        public int Goals;
        public int Assists;
        public int Passes;
        public int MatchesPlayed;
    }

    class FootballPlayer
    {
        public CurrentMatchDetails CurrentMatchDetails { get; set; }
        public SeasonDetails SeasonDetails { get; set; }
        public CareerDetails CareerDetails { get; set; }
        public GeneralDetails GeneralDetails { get; set; }
    }

A few things I do not like about this

• I have to make the classes (the SeasonDetails, CareerDetails etc) public
• I have to instantiate all of these classes separately after instantiating the FootballPlayerClass

But I am not sure if it is the best practice. I was thinking of creating a embedded class within the class FootballPlayer.

I am going to be using the class in a WPF application and going to implement INotifyPropertyChanged on my FootballPlayer class. By using the method above, I am going to have to imlpement INPC on all of the classes such as CareerDetails etc. So what should I do instead or should I stick with what I have?

I may have another base class called 'FootballTeam' which could have a sub-class named CurrentMatchDetails as well - It may look like this

    class CurrentMatchDetails
    {
        public double TimeElapsed;
        public string RefereeName;
    }

so I should be able to access properties like this teamObject.CurrentMatchDetails.RefereeName or playerObject.CurrentMatchDetails.Goals;

Answer 1

I may have another base class called 'FootballTeam' which could have a sub-class named CurrentMatchDetails as well

First, a class named FootBallTeam should never have a subclass named CurrentMatchDetails. Inheritance is not a code sharing mechanism, but to model according to physical world. Is CurrentMatchDetails a FootballTeam? I see no way. Inheritance work for this kind of model:

class FootballPlayer
{

}

class StarPlayer : FootballPlayer
{

}

Here StarPlayer is a FootballPlayer, so all those properties of a football player should be available on a star player too. You should use composition, please read more here: Prefer composition over inheritance?. In your case FootBallTeam should be a property on the CurrentMatchDetails.

I dont have a better answer than Ed's (+1 for him), I will just try to flesh out his a bit further.

public class PlayerStat //Add 'Player' to name since stats can be for tourneys too
{
    public int Goals { get; set; }
    public int Assists { get; set; }
    public int Passes { get; set; }
}

public class PlayerCurrentMatchStat : PlayerStat 
{
    public bool IsCaptain { get; set; }
}


public class PlayerSeasonStat : PlayerStat 
{
    public int MatchesPlayed { get; set; }
}

public class PlayerCareerStat : PlayerStat 
{

}


//And your football player goes like this:
class FootballPlayer
{
    public FootballPlay()
    {
        CurrentStats = new CurrentStat();
        SeasonStats = new SeasonStat();
        CareerStats = new CareerStat();
    }

    public string Name { get; set; }
    public string TeamName { get; set; }   
    public PlayerCurrentMatchStat CurrentMatchStat { get; set; }
    public PlayerSeasonStat  SeasonStat { get; set; } //no need of naming 'Player' 
    public PlayerCareerStat CareerStat { get; set; }  //inside Player class
}

If visibility of some classes are bothering you, you can protect it by pushing them to some specific namespace. And finally a football team is still perfectly valid entity even if they are not in a match. Model it this way:

class FootballTeam
{
    // all what makes a good unit
}    

class FootballMatch
{
    public FootBallTeam Team1 { get; set; } 
    public FootBallTeam Team2 { get; set; } 
    public double TimeElapsed { get; set; }   
    public string RefereeName { get; set; }   
}

Makes sense. Now call it like:

FootballMatch currentMatch = new FootballMatch(...whatever

---

Alert: The design above is ok but still a bit smelly. Why would a FootballPlayer have a PlayerCurrentMatchStat? A football player is still a perfect player even if he is in the bench. May be you could make it null if he is not in a match. Same goes for PlayerSeasonStat - if so which season? I would completely redesign it, and imo is a little more complex, but a lot more logical.

public interface Stat //common interface which can be for tourneys, matches etc too
{
    int Goals { get; set; }
    int Assists { get; set; }
    int Passes { get; set; }
}
//You might want to break this interface further to fit in other specific stats
//like player stat, match stat etc.
//Also making it an interface rather than base class is better, you shouldnt have 
//2 different entities like PlayerStat and MatchStat share a common base class

public class PlayerStat : Stat //can be career stat, single match stat etc;
{                              //depends on context where you call it
    public int Goals { get; set; }
    public int Assists { get; set; }
    public int Passes { get; set; }
}

//another example
public class MatchStat : Stat 
{
    public int Goals { get; set; }
    public int Assists { get; set; }
    public int Passes { get; set; }
    public int Viewership { get; set; }
}



class Team
{
    // all what makes a good unit
}    

class Match
{
    public Team Team1 { get; set; } 
    public Team Team2 { get; set; } 
    public double TimeElapsed { get; set; }   
    public string RefereeName { get; set; } 
    public MatchStat Stat { get; set; } //a match has match stat
}



//provide a suitable constructor that takes a player and a match
public class PlayerMatchStat
{
    public Player Player { get; set; }
    public Match Match { get; set; }
    public PlayerStat Stat { get; set; } //should return player's stat for this match
    public bool IsCaptain { get; set; }
    public int DistanceCompleted { get; set; }
}

//provide a suitable constructor that takes a player and a season
public class PlayerSeasonStat
{   
    public bool Player { get; set; }
    public Season Season { get; set; } //have a season class
    public PlayerStat Stat { get; set; } //should return player's stat for this season
    public int RedCards { get; set; }
}


//and lastly
class Player
{
    public Player()
    {
        //init work
    }

    public string Name { get; set; } 
    public PlayerStat CareerStat { get; set; } //just like match has match stat
    public Team[] Teams { get; set; } //Team name shouldn't go within a player 
                                      //class, he could be in many teams.

    //since player inherently has no match or season, we shall provide methods to query them
    public PlayerMatchStat GetMatchStat(Match match)
    {
        return new PlayerMatchStat(match, this);
    }
    public PlayerSeasonStat GetSeasonStat(Season season)
    {
        return new PlayerSeasonStat(season, this);
    }
}

Here inheritance is used sparingly (where it warrants) and what is used to good effect is composition.

Answer 2

You should create a StatDetails object:

class FootballPlayer
{
    public FootballPlay()
    {
        CareerStats = new StatDetails();
        SeasonStats = new StatDetails();
        CurrentStats = new StatDetails();
    }

    public string Name { get; set; }
    public string TeamName { get; set; }

    public StatDetails CareerStats { get; set; }
    public StatDetails SeasonStats { get; set; }
    public StatDetails CurrentStats { get; set; }
}

class StatDetails
{
    public int Goals { get; set; }
    public int Assists { get; set; }
    public int Passes { get; set; }
}

In this way, you only have to implement INotifyPropertyChanged on two classes.