Best way to use a base class, is this right?

Question

I am coding an application that I thought would be a good chance to use a base class. I have Player class which holds an instance for each player on my game, I also have a PlayerManager class that has a dictionary of all the connected players, although I'll leave the PlayerManager class out of this question as this is just about the Player and PlayerData class.

So, I thought instead of having something like this, please note before checking this code snippet that I have removed a lot of the code and just shown a minimal example.

class Player
{
    public PlayerData;
}

class PlayerData
{
    public string Username;
    public string Motto;
    public string NickName;
}

class SomeOtherClass
{
    public void Test()
    {
        var player = GetPlayer();

        Console.WriteLine("Hello, I am " + player.PlayerData.Username);
    }
}

I thought why have a method when I can have a base class? So I thought great, lets use a base class, this is what I ended up with.

internal class Player : PlayerData, IDisposable
{
    private readonly Socket _socket;

    private bool _disposeCalled;
    private bool _receivingData;
    private bool _hasAuthenticated;

    public Player(Socket socket)
    {
        _socket = socket;
    }

    public void OnAuthenticated(MySqlDataReader reader)
    {
        if (_hasAuthenticated)
        {
            return;
        }

        _hasAuthenticated = true;
        AssignData(reader);
    }

    public void Dispose()
    {
        if (_disposeCalled)
        {
            return;
        }

        _disposeCalled = true;

        if (_receivingData)
        {
            _receivingData = false;

            try
            {
                if (_socket != null && _socket.Connected)
                {
                    _socket.Shutdown(SocketShutdown.Both);
                    _socket.Close();
                }
            }
            catch
            {
                // ignored
            }

            _socket?.Dispose();
        }

        if (_hasAuthenticated)
        {
            SaveData();
        }
    }
}

internal class PlayerData
{
    public int Id;
    public string Username;

    public void AssignData(MySqlDataReader reader)
    {
        while (reader.Read())
        {
            Id = reader.GetInt32("id");
            Username = reader.GetString("username");
        }
    }

    public void SaveData()
    {
        using (var dbConnection = Program.Server.Database.Connection)
        {
            dbConnection.SetQuery("UPDATE `users` SET `username` = @username WHERE `id` = @id");
            dbConnection.AppendParameter("id", Id);
            dbConnection.AppendParameter("username", Username);
            dbConnection.ExecuteNonQuery();
        }
    }
}

You'll probably see the base class has a constructor, that's because I was going to just pass the PlayerData's data with the Player's constructor, but I wont actually get the data untill the Player's class has been fully initialized, I don't know when that will be as its done via socket packets, I just assign the data when I notice its been authenticated.

The point of my question is, should I use a base class like this, or should I not use a base class due to the fact I'm not initializing the data via the constructor, or is it okay to assign it via another method later on? Do I really need a base class, am I not following the right official rules for what a base class is and used for? Basically I just want to know, with this call stack should I be using a base class or method? I'm unsure on the rules.

Answer 1

So, the rules are not rules so much as guidelines (that not everyone really agrees on).

THAT SAID, I don't really see any benefit of lumping these classes together, although that could be because of so much of the code being removed.

In general, a good rule of thumb is not to use inheritance unless you have a good reason--a significant amount of code reuse that couldn't be achieved through composition, for example. In most cases, your code will be easier to maintain if you design with an eye for reducing dependencies (coupling), and the dependency between a subclass and its superclass is very strong. This means keeping things separate.

One technique you could do to simplify calls to player data is to "promote the interface"--basically, add the methods/properties you want as a sort of facade on the Player class, and have that relay the code to its PlayerData object. This has a few benefits: 1. It hides dependencies on PlayerData, which means you are free to change implementation to consolidate or use a different type (for example, if you just wanted to put those values in a data structure in the Player class) 2. It allows you to handle the case where PlayerData is being requested, but hasn't been initialized yet. For example, you could return default values or throw a custom exception. 3. Player and PlayerData are free to vary independently. So, if you run across a valid reason to subclass one or both of them, you won't be constrained.

In summary, it doesn't look like you really gain much from using inheritance in this way, but it would cut off design choices down the road. Also, anytime you are using inheritance to describe a relationship that is not an "IS A" relationship (PlayerData Is A Player? nope)--that smells fishy.

Again, guidelines. Bottom line is you want to design a system that you want to maintain, and design decisions come with trade-offs (often between simplicity and flexibility). So, if you decide there is a good reason to keep this as a subclass, just document it and don't worry about the OO Police coming after you for breaking the rules ;)