IoC.Resolve deep in the project or Constructor Injection on entry point of app

Question

I'm building cross-platform application (every mobile/desktop OS + web in ASP.NET). I'm using Xamarin, so I can do everything in C#. But before I start working on UIs, I want to build soild, flexible Core/Domain project that will be shared in all Client apps.

I read a lot about DI and IoC containers, I know a lot of theory, but when it comes to coding I'm confused and sometimes I've a feeling I have no idea what am I doing.

Quickly about my app: It will be simple game, like tic tac toe. But I want to do it as flexible as possible, so it will be easy to build on top of it another games, like: connect four or reversi or even chess. Simple 2 players board games.

My concerns are about my "DI-friendly" implementation and the way of using IoC container. My project has 3 main layers: UI<-API<-Domain(Entites). In UI I need to create IGame object. Game has its GameType (just an enum), List<IPlayers> and IBoard.

My question(s) are:

• Should I resolve my interfaces in one place? Good example is IBoard in my project. It is needed and used only deep in Game object. Game obviously depends on IBoard. So as I read it'd be good practice to inject IBoard through constoructor - so it's clear: "Game depends on Board". But this way, I create Board instance in UI, then I have to pass it to GameAPI, then to GameFactory and finally to Game object. I could easily create it or resolve it in Game constructor, without passing through every layer in application.

• Where exactly should I resolve my interfaces? Should it be on very top in UI? Should it be in one place? Where should I create/resolve my IBoard?

I did't want to paste here fragments of code and force you to paste it back to IDE, so I've made "Short, Self Contained, Correct (Compilable), Example" and put it here.

Here's my UI:

namespace Tictactoe.UI
{
    public partial class Form1 : Form
    {   
        private  GameAPI GameAPI { get; } = new GameAPI();
        private  IGame CurrentGame { get; set; }
        IKernel kernel = new StandardKernel();    

        public Form1()
        {
            InitializeComponent();    

            // Load all dependencies
            var modules = new List<INinjectModule> { new TictactoeModule() };
            kernel.Load(modules);
        }    

        private void buttonNewGameSingle_Click(object sender, EventArgs e)
        {
            StartNewGame(GameType.SinglePlayer);
        }    

        private void buttonNewTwoPlayers_Click(object sender, EventArgs e)
        {
            StartNewGame(GameType.TwoPlayers);
        }    

        private void buttonNewGameOnline_Click(object sender, EventArgs e)
        {
            StartNewGame(GameType.Online);
        }    

        private void StartNewGame(GameType type)
        {
            EnableBoard(true);
            ResetBoard();    

            // Without IoC
            //IPlayerFactory playerFactory = new PlayerFactory(); // Resolve player facotry
            //IGameFactory gameFactory = new GameFactory(playerFactory); // Resolve game facotry
            //IBoard board = new Board(3,3); // Resolve board    

            // With IoC
            IPlayerFactory playerFactory = kernel.Get<IPlayerFactory>();
            IGameFactory gameFactory = kernel.Get<IGameFactory>(new ConstructorArgument("playerFactory", playerFactory));
            IBoard board = kernel.Get<IBoard>(new ConstructorArgument("width", 3), new ConstructorArgument("height", 3));    

            CurrentGame = GameAPI.CreateGame(gameFactory, type, board);
        }    

        private void button_Click(object sender, EventArgs e)
        {
            Button clicked = sender as Button;
            if (clicked != null)
            {
                int[] xy;
                buttons.TryGetValue(clicked.Name, out xy);    

                Console.WriteLine(clicked.Name + "[" + xy[0] + "," + xy[1] + "]");

                if (CurrentGame.IsMoveValid(xy[0], xy[1]))
                {
                    IMove result = CurrentGame.MakeMove(xy[0], xy[1]);
                    clicked.Text = result.PlayerId.ToString();
                    if (result.IsConnected)
                    {
                        EnableBoard(false);
                        MessageBox.Show("Finished! Player " + result.PlayerId + " won!");
                    }
                    else if (result.IsGameOver)
                    {
                        EnableBoard(false);
                        MessageBox.Show("Game over!!");
                    }
                }
            }
        }    

        #region UI Helpers    

        private Dictionary<string, int[]> buttons = new Dictionary<string, int[]>
        {
            { "button1", new[] {0, 0} },
            { "button2", new[] {0, 1} },
            { "button3", new[] {0, 2} },
            { "button4", new[] {1, 0} },
            { "button5", new[] {1, 1} },
            { "button6", new[] {1, 2} },
            { "button7", new[] {2, 0} },
            { "button8", new[] {2, 1} },
            { "button9", new[] {2, 2} }
        };    

        private void EnableBoard(bool enable)
        {
            button1.Enabled = enable;
            button2.Enabled = enable;
            button3.Enabled = enable;
            button4.Enabled = enable;
            button5.Enabled = enable;
            button6.Enabled = enable;
            button7.Enabled = enable;
            button8.Enabled = enable;
            button9.Enabled = enable;
        }    

        private void ResetBoard()
        {
            button1.Text = string.Empty;
            button2.Text = string.Empty;
            button3.Text = string.Empty;
            button4.Text = string.Empty;
            button5.Text = string.Empty;
            button6.Text = string.Empty;
            button7.Text = string.Empty;
            button8.Text = string.Empty;
            button9.Text = string.Empty;
        }    

        #endregion    


    }
}    

Here's my DependencyResolver:

namespace Tictactoe.DependencyResolver
{
    public class TictactoeModule : NinjectModule
    {
        public override void Load()
        {
            Bind<IPlayerFactory>().To<PlayerFactory>();
            Bind<IGameFactory>().To<GameFactory>();
            Bind<IBoard>().To<Board>();
        }
    }
}

Here's my API. UI uses only API methods.

namespace TicTactoe.API
{
    public class GameAPI
    {
        public IGame CurrentGame { get; set; }    

        public IGame CreateGame(IGameFactory gameFactory, GameType type, IBoard board)
        {
            CurrentGame = gameFactory.CreateGame(type, board);
            return CurrentGame;
        }    

        public IMove MakeMove(int row, int column)
        {
            if (CurrentGame == null) return null;
            return CurrentGame.MakeMove(row, column);
        }    

        //TODO: Online player move, or bot
    }
}

And finally, my Domain. My interfaces:

    public interface IBoard
    {
        int Width { get; set; }
        int Height { get; set; }
        int[,] Fields { get; set; }
        int WinnerId { get; set; }    

        void Reset();
        bool IsMoveValid(int row, int column, int playerId);
        IMove InsertChip(int row, int column, int playerId);
    }    

    public interface IGame
    {
        GameType Type { get; }
        IBoard Board { get; }
        List<IPlayer> Players { get; }    

        bool IsMoveValid(int row, int column);
        IMove MakeMove(int row, int column);
    }    

    public interface IGameFactory
    {
        IGame CreateGame(GameType type, IBoard board);
    }    

    public interface IMove
    {
        int PlayerId { get; set; }
        bool IsConnected { get; }
        bool IsGameOver { get; }
    }    

    public interface IPlayer
    {
        int Id { get; set; }
        string Name { get; set; }
        PlayerType Type { get; set; }
    }    

    public interface IPlayerFactory
    {
        IPlayer CreatePlayer(PlayerType type, int id);
    }        

And my implementation in Domain:

namespace Tictactoe.Domain
{
    public class Game : IGame
    {
        private int currentPlayerIndex = 1;    

        public Game(GameType type, IBoard board, List<IPlayer> players)
        {
            Type = type;
            Board = board;
            Players = players;
        }    

        public GameType Type { get; }
        public IBoard Board { get; }
        public List<IPlayer> Players { get; }    

        public bool IsMoveValid(int row, int column)
        {
            // It could be done entierly here. Just doing what Board now is doing: if (Board.Fields[row, column] == 0) return true; 
            // But there can be much more things to do in more complicated games, so I'm not sure about that
            // I left that job in Board class, and just call it.    

            if (Players[currentPlayerIndex - 1].Type.Equals(PlayerType.Human))
            {
                return Board.IsMoveValid(row, column, currentPlayerIndex);
            }    

            //if current player is Bot or Online player, your move is not allowed
            return false;
        }    

        public IMove MakeMove(int row, int column)
        {
            IMove result = Board.InsertChip(row, column, currentPlayerIndex);    

            // Set next player
            if (!result.IsConnected) currentPlayerIndex = currentPlayerIndex == 1 ? 2 : 1;    

            return result;
        }
    }
}    

namespace Tictactoe.Domain
{
    public class Board : IBoard
    {
        public int Width { get; set; }
        public int Height { get; set; }
        public int[,] Fields { get; set; }
        public int WinnerId { get; set; }    

        public Board(int width, int height)
        {
            Width = width;
            Height = height;
            Reset();
        }    

        public void Reset()
        {
            Fields = new int[Width,Height];
            for (int i = 0; i < Fields.GetLength(0); i++)
            {
                for (int j = 0; j < Fields.GetLength(1); j++)
                {
                    Fields[i, j] = 0;
                }
            }
        }    

        public bool IsMoveValid(int row, int column, int playerId)
        {
            // There will much more work to do in more complicated games
            if (Fields[row, column] == 0) return true;
            return false;
        }    

        public IMove InsertChip(int row, int column, int playerId)
        {
            bool success = false;
            bool gameOver = true;
            Fields[row, column] = playerId;    

            // Very bad, I know. But it's just quick temp solution:
            if (Fields[0, 0].Equals(playerId) && Fields[0, 1].Equals(playerId) && Fields[0, 2].Equals(playerId)) success = true;
            if (Fields[1, 0].Equals(playerId) && Fields[1, 1].Equals(playerId) && Fields[1, 2].Equals(playerId)) success = true;
            if (Fields[2, 0].Equals(playerId) && Fields[2, 1].Equals(playerId) && Fields[2, 2].Equals(playerId)) success = true;    

            if (Fields[0, 0].Equals(playerId) && Fields[1, 0].Equals(playerId) && Fields[2, 0].Equals(playerId)) success = true;
            if (Fields[0, 1].Equals(playerId) && Fields[1, 1].Equals(playerId) && Fields[2, 1].Equals(playerId)) success = true;
            if (Fields[0, 2].Equals(playerId) && Fields[1, 2].Equals(playerId) && Fields[2, 2].Equals(playerId)) success = true;    

            if (Fields[0, 0].Equals(playerId) && Fields[1, 1].Equals(playerId) && Fields[2, 2].Equals(playerId)) success = true;
            if (Fields[0, 2].Equals(playerId) && Fields[1, 1].Equals(playerId) && Fields[2, 0].Equals(playerId)) success = true;    

            // Check if there any valid move has left
            if (Fields.Cast<int>().Any(field => field == 0)) gameOver = false;

            return new Move(playerId, success, gameOver);
        }
    }
}    

namespace Tictactoe.Domain
{
    // Separate class for Move result, because in more compilcated games, 
    // there could be more info to pass.
    // Even here, UI would like to know how fields are connected (when won) - to show it (painting the line or sth)
    public class Move : IMove
    {
        public int PlayerId { get; set; }
        public bool IsConnected { get; }
        public bool IsGameOver { get; }    

        public Move(int playerId, bool isConnected, bool isGameOver = false)
        {
            PlayerId = playerId;
            IsConnected = isConnected;
            IsGameOver = isGameOver;
        }
    }
}    

namespace Tictactoe.Domain
{
    public class Player : IPlayer
    {
        public int Id { get; set; }
        public string Name { get; set; }
        public PlayerType Type { get; set; }    

        public Player(PlayerType type, int id)
        {
            Type = type;
            Id = id;
        }
    }
}

Answer 1

"Should I resolve my interfaces in one place? "

Yes. This is called a composition root. You should definitely not use your IOC container as a service locator. This is considered an antipattern and also really just removes the point of inversion of control; instead of getting dependency injection, you're really just creating an abstracted monolithic factory instead.

The article I linked above (here again just in case) actually has a lot of good information on how you can abuse dependency injection frameworks as service locators and why it is a bad idea, but here's a relevant excerpt from it..

public class OrderProcessor : IOrderProcessor
{
    public void Process(Order order)
    {
        var validator = Locator.Resolve<IOrderValidator>();
        if (validator.Validate(order))
        {
            var shipper = Locator.Resolve<IOrderShipper>();
            shipper.Ship(order);
        }
    }
}

Okay, so the class has a default constructor. That means I can simply create a new instance of it and invoke the Process method right away:

var order = new Order();
var sut = new OrderProcessor();
sut.Process(order);

Alas, running this code surprisingly throws a KeyNotFoundException because the IOrderValidator was never registered with Locator. This is not only surprising, it may be quite baffling if we don't have access to the source code.

Answer 2

My emphasis:

Where exactly should I resolve my interfaces? Should it be on very top in UI? Should it be in one place? Where should I create/resolve my IBoard?

The UI is only at the very top if you put it there because a WinForms application template normally looks like this:

static class Program
{
    /// <summary>
    /// The main entry point for the application.
    /// </summary>
    [STAThread]
    static void Main()
    {
        Application.EnableVisualStyles();
        Application.SetCompatibleTextRenderingDefault(false);
        Application.Run(new Form1());
    }
}

This code is exactly like this, untouched, unchanged, in your GitHub repository.

If there's one place to implement a composition root, it's right here - that's the actual entry point, it doesn't get any higher-to-the-top than that. It's where the first new statement creates the first object in the application, a Form1 instance. Form closes, Main reaches its closing brace, program ends.

There's a dependency on winforms here, with the System.Windows.Forms.Application. Then with the Form type Form1 is derived from.

If this is our composition root, then we need 3 things to happen:

• Register
• Resolve
• Release

"Register" is the configuration part, which you correctly handled in NinjectModule objects.

"Resolve" is when you call Get<T>() to fetch an instance of something from the container.

"Release" has been overlooked, but it's no big deal since no IDisposable object is involved here. Well, except the IKernel itself, which you're not disposing, but then, when the form closes the app terminates anyway. Still, best practice would be to dispose of disposables.

Ayway you're doing the "register" part in the constructor:

public Form1()
{
    InitializeComponent();

    // Load all dependencies
    var modules = new List<INinjectModule> { new TictactoeModule() };
    kernel.Load(modules);
}

Which means kernel.Load(modules); is called by the WinForms designer. There's a reason why a Form needs a public parameterless constructor that calls InitializeComponent: the designer calls it.

Here there's a new StandardKernel() being instantiated, so Form1 is tightly coupled with StandardKernel, no matter how pretty having an IKernel private field looks.

So how do you constructor-inject anything into a Form without breaking the designer? You chain your parameterized constructor with the default/parameterless one.

private readonly IKernel _kernel; // smells...

/// <summary>
/// Default constructor required for designer.
/// </summary>
public Form1()
{
    InitializeComponent();
}

/// <summary>
/// Default constructor required for designer.
/// </summary>
public Form1(IKernel kernel) // ah-ha! service locator!
    : this() // need to ensure default constructor runs...
             // ...the alternative is to duplicate the InitializeComponent() call.
{
    _kernel = kernel; 
}

Notice that the type is now receiving the IoC container as a dependency, through its constructor. Yes, as an abstraction/interface, but the thing with passing the IoC container around as a dependency, is that you've just defeated the entire purpose of DI/IoC: you've leaked the dependency on your IoC container into your application, and now you have business code that needs an IoC container to work.

Dependency Injection doesn't need an IoC container. Let's see what we're resolving with that kernel.

// Without IoC
//IPlayerFactory playerFactory = new PlayerFactory(); // Resolve player facotry
//IGameFactory gameFactory = new GameFactory(playerFactory); // Resolve game facotry
//IBoard board = new Board(3,3); // Resolve board

// With IoC
IPlayerFactory playerFactory = kernel.Get<IPlayerFactory>();
IGameFactory gameFactory = kernel.Get<IGameFactory>(new ConstructorArgument("playerFactory", playerFactory));
IBoard board = kernel.Get<IBoard>(new ConstructorArgument("width", 3), new ConstructorArgument("height", 3));

Interesting. Which one is better? Neither. That's not how you use an IoC container. If you've read the theory of DI/IoC, you must have read about...

Don't call them, they'll call you.

The Hollywood Principle

Well an IoC container is like Hollywood. You don't call an IoC container. It calls you. Sounds backwards? That's why it's called inversion of control!

Code that does DI correctly, could be recompiled with a new composition root that's using a new IoC framework, and there wouldn't be anything to change in the business code. Nothing. Not even a single instruction.

Unless you've registered them InSingletonScope(), you're resolving a new IPlayerFactory and a new IGameFactory instance every time a new game starts; resolving stands for resolving dependencies, so Form1 has dependencies on IGameFactory and IPlayerFactory. But hold on:

CurrentGame = GameAPI.CreateGame(gameFactory, type, board);

The IGameFactory instance is actually a dependency of the GameAPI class!

private  GameAPI GameAPI { get; } = new GameAPI();

There: GameAPI is another type that's strongly coupled with Form1. And this new means something isn't right. Every time you new something up, you fail Dependency Injection.

The GameAPI class has no constructor, but exposes this CreateGame method:

public IGame CurrentGame { get; set; }

public IGame CreateGame(IGameFactory gameFactory, GameType type, IBoard board)
{
    CurrentGame = gameFactory.CreateGame(type, board);
    return CurrentGame;
}

So you have a reference to the CurrentGame in two places - in Form1, and then in GameAPI. And Form1 has a dependency on GameAPI, so you could do this:

CurrentGame = GameAPI.CreateGame(gameFactory, type, board);

Or that:

private readonly GameAPI _api = new GameAPI();
private IGame CurrentGame { get { return _api.CurrentGame; } }

Or, actually, just this:

private readonly GameAPI _api = new GameAPI();

And use _api.CurrentGame instead of having two references to the same object.

But the main problem I'm seeing here is that MakeMove is also calling into CurrentGame: the class is just an excuse for that CreateGame method that not only creates an IGame object and returns it, it also sets the CurrentGame as an undocumented side-effect.

Get rid of that GameAPI class, it smells.

So what should the Form1 constructor look like then? What are the real dependencies?

• IPlayerFactory is only ever used as a constructor argument for IGameFactory.
• IGameFactory is only ever used to create an IGame.
• IBoard is only ever used as a parameter for the IGameFactory.

The StartNewGame method is called by 3 of the buttons on the form, each passing in a different GameType enum value.

"Start new game" sounds an awful lot like a job for a game factory - well done! Looking at the usages for the IGame CurrentGame it creates, the game is actually implemented inside a button_Click handler - but I'll get back to that. I was talking about the constructor.

The factories could be simplified.

At first glance the dependency on IPlayerFactory isn't real, and GameType isn't really needed either. Consider:

public class GameFactory : IGameFactory
{
    public IGame Create(IBoard board, IEnumerable<IPlayer> players)
    {
        return new Game(board, players);
    }
}

Turns out a factory that's as simple as that can be 100% handled by Ninject's Factory extension, like this:

Bind<IGameFactory>().ToFactory();

Boom. Done. At runtime you get a proxy type that implements IGameFactory and passes the Create method's parameters into the constructor of whatever type is registered for an IGame: you never ever need to actually implement an Abstract Factory with a concrete type ever again.

It would also work if the GameFactory class had an IPlayerFactory dependency like your implementation does, but it's a false dependency that is obscuring things - and that's how you get an IGameFactory that returns IPlayer objects.

Your IPlayerFactory itself is flawless - I would only have left it to Ninject to implement the concrete type on the fly. The problem isn't the IPlayerFactory, it's where the dependency is. It makes no sense for an IGameFactory to create anything other than IGame objects; that's why I'm leaving it up to whoever is calling Create to supply an IEnumerable<IPlayer>, because you don't really care what IGame.Type is - you're exposing it as a getter, but it's not used anywhere, it just pollutes the IGame interface.

So, that constructor.

private readonly IGameFactory _gameFactory;

public Form1(IGameFactory gameFactory, IPlayerFactory playerFactory) : this()
{
    _gameFactory = gameFactory; 
    _playerFactory = playerFactory;
}

Now the button click handlers don't need to pass a GameType anymore, they just call _playerFactory.Create to create the players required, and then pass the created players to _gameFactory.Create to create the IGame, at least for now.

---

Register.

The registration code could be simplified, using the Ninject.Extensions.Conventions extension - that way you can give yourself a naming convention and define it, and then whatever interfaces you add to your project are automatically handled. For example you could make a convention that says "bind all interfaces for all classes in all assemblies of the solution except those with a name ending with 'Factory'" -- and then make another convention that says "bind all interfaces with a name ending with 'Factory' to a Ninject factory.", like this:

var assemblies = new[]
{
    Assembly.GetExecutingAssembly(),
    Assembly.GetAssembly(typeof(IGame)),
};

Bind<Form1>().ToSelf();

Bind(t => t.From(assemblies)
           .SelectAllClasses()
           .Where(type => !type.Name.EndsWith("Factory"))
           .BindAllInterfaces());

Bind(t => t.From(assemblies)
           .SelectAllInterfaces()
           .Where(type => type.Name.EndsWith("Factory"))
           .BindToFactory());

Rebind<IBoard>().To<Board>()
                .WithConstructorArgument("width", 3)
                .WithConstructorArgument("height", 3);

And then you can pretty much leave that NinjectModule implementation alone for a while ;-)

Resolve.

The composition root would look like this:

static class Program
{
    /// <summary>
    /// The main entry point for the application.
    /// </summary>
    [STAThread]
    static void Main()
    {
        var kernel = new StandardKernel();
        kernel.Load(new TicTacToeModule()); // register

        var mainForm = kernel.Get<Form1>(); // resolve
        Start(mainForm);

        kernel.Dispose(); // release
    }

    static void Start(Form mainForm)
    {
        Application.EnableVisualStyles();
        Application.SetCompatibleTextRenderingDefault(false);
        Application.Run(mainForm);
    }
}

As you see, there's really only 1 kernel.Get<T> call you need to make, to resolve the mainForm object and with it, the entire dependency graph of the application. Here's the same code without an IoC container:

static class Program
{
    /// <summary>
    /// The main entry point for the application.
    /// </summary>
    [STAThread]
    static void Main()
    {
        var board = new Board(3, 3);
        var gameFactory = new GameFactory(board);
        var playerFactory = new PlayerFactory();
        var mainForm = new Form1(gameFactory, playerFactory);

        Start(mainForm);
    }

    static void Start(Form mainForm)
    {
        Application.EnableVisualStyles();
        Application.SetCompatibleTextRenderingDefault(false);
        Application.Run(mainForm);
    }
}

A few points:

• All newing up is happening at the composition root whether there's an IoC container or not.
• An application that size hardly benefits from an IoC container - "poor man's DI" makes code easier to follow; introduce an IoC container when manually newing things up becomes more painful than configuring the container.

---

I'll stop here for this answer, but refactoring of the code should go further than that: the logic that's in the click handlers belongs in the Game class, which means the factories probably belong there as well. But when you get to that point, you'll be ready for a new follow-up post anyway.

Don't give up!