3
\$\begingroup\$

I have coded a system where each 'Player' class requires 5 components (could be more in the future, up to 10, 20 maybe even 30, once we add more and more features). What I want some advice on is a better way of creating these components, I am not saying that my current way is bad, I am just looking for anyone who could throw a fresh idea my way, to give my application a much better feel component wise.

My current way:

_searchesComponent = new PlayerSearchComponent(_playerConnection);
_searchesComponent.InitializeComponent();

_ignoreComponent = new PlayerIgnoreComponent(_playerConnection);
_ignoreComponent.InitializeComponent();

_effectsComponent = new PlayerEffectComponent(_playerConnection);
_effectsComponent.InitializeComponent();

_itemComponent = new PlayerItemComponent(_playerConnection);
_itemComponent.InitializeComponent();

_logComponent = new PlayerLogComponent(_playerConnection);
_logComponent.InitializeComponent();

Here is the base layout of one of my component classes:

namespace MyApp.Players.Players.Effects
{
    using System;
    using System.Collections.Generic;
    using Components;
    using NLog;

    internal sealed class PlayerIgnoreComponent : IDisposable, IComponent
    {
        private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

        private readonly PlayerConnection _playerConnection;
        private List<int> _ignores;

        public PlayerIgnoreComponent(PlayerConnection playerConnection)
        {
            _playerConnection = playerConnection;
            _ignores = new List<int>();
        }

        public void InitializeComponent()
        {
            using (var databaseConnection = Server.Database.NewDatabaseConnection)
            {
                var playerId = _playerConnection.SelectColumnInt("id");

                databaseConnection.SetQuery("SELECT * FROM `user_ignores` WHERE `user_id` = @userId");
                databaseConnection.AppendParameter("userId", playerId);

                using (var reader = databaseConnection.ExecuteReader())
                {
                    while (reader.Read())
                    {
                        _ignores.Add(reader.GetInt32("ignore_id"));
                    }
                }
            }
        }

        public void Dispose()
        {
            Dispose(true);
        }

        private void Dispose(bool disposing)
        {
            if (!disposing)
            {
                return;
            }

            _ignores.Clear();
            _ignores = null;
        }
    }
}

Fields:

private PlayerConnection _playerConnection;
private PlayerEffectComponent _effectsComponent;
private PlayerSearchComponent _searchesComponent;
private PlayerIgnoreComponent _ignoreComponent;
private PlayerClothingComponent _clothingComponent;

Here is IComponent:

namespace MyApp.Players.Players.Components
{
    public interface IComponent
    {
        void InitializeComponent();
    }
}

Any performance enhancement comments, answers or anything just to overall improve the code is also welcome.

\$\endgroup\$
  • \$\begingroup\$ Why are the fields listed in a separate code block? Can the question be edited to include the class module as it appears in your IDE? \$\endgroup\$ – Mathieu Guindon Mar 20 '17 at 14:52
  • 1
    \$\begingroup\$ Use a DI container \$\endgroup\$ – Mark Brackett Mar 20 '17 at 15:00
  • \$\begingroup\$ Please post all of two. Right not you do nothing with private List<int> _ignores; \$\endgroup\$ – paparazzo Mar 20 '17 at 15:52
3
\$\begingroup\$

Because the class is sealed, your IDisposable implementation is over-complicated. The whole entire point of the disposing flag is to make sure unmanaged resources get cleaned up correctly even if a class inherits this type and overrides Dispose... but your method isn't virtual, so it's only ever called/callable with Dispose(true).

Scrap that, keep it simple:

public void Dispose()
{
    _ignores.Clear();
    _ignores = null;       
}

Actually, if you're using that type correctly, then you gain nothing from explicitly clearing the internal list and nulling it up - the instance is [in theory] about to get collected anyway; IDisposable is making your class much more complex than it needs to be.

Typically you'd make a class implement IDisposable when it's holding on at instance level to a reference that implements IDisposable itself, which does not appear to be the case here (unless PlayerConnection has implications that aren't in your post) - disposing a List<int> is pure overkill.

Moreover, calling Dispose twice should have no ill effect on the calling code, but in your case it throws a rather surprising NullReferenceException, and then you could very well call InitializeComponent again... without getting an expected ObjectDisposedException (you get another NullReferenceException instead).

The IComponent interface seems uncalled for, given how the callers seem to only be calling the concrete types' implementations of the InitializeComponent method.

The implementation is tightly coupled with some static/global-scope database-interacting service, which makes it untestable.


If all "components" load things from a database, it seems you need to look into the repository pattern, and possibly the unit of work as well. Consider using dependency injection to decouple your implementation from its dependencies, and avoid static / global-scope objects in OOP as a general rule of thumb.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I'd argue the disposing (not disposed) flag is not for inheritance but to differentiate between when it's called manually and when called from the finalizer thread, to alter the method's logic. Hence the standard pattern of Dispose(true) when called manually, and Dispose(false) from the destructor. Of course in this case it's irrelevant since it is only ever called manually (if at all). \$\endgroup\$ – 404 Mar 20 '17 at 15:53
  • \$\begingroup\$ @eurotrash correct - but there's no finalizer involved either; and the Dispose Pattern is pretty clear: "DO declare a protected *virtual* void Dispose(bool disposing) method to centralize all logic related to releasing unmanaged resources." - that virtual overload is specifically so inherited types can hook into the object's disposal without accidentally leaving stuff alive in the base class. The parameterized overload is virtual in every implementation I've seen, which makes it irrelevant for a sealed class. \$\endgroup\$ – Mathieu Guindon Mar 20 '17 at 16:00
  • 1
    \$\begingroup\$ Hmm you're right. Got to say that when I implement Dispose(bool disposing) I always make it private (can't remember ever implementing a Disposable class where I wanted to allow inheritance, much less allow Dispose to be overridable). Useful page on the Dispose pattern, bookmarked. \$\endgroup\$ – 404 Mar 20 '17 at 17:05
0
\$\begingroup\$

Don't SELECT * if you are only using one column

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @Tushar Don't SELECT * if you are only using one column \$\endgroup\$ – paparazzo Mar 20 '17 at 15:49

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.