Client Server Stocks application

Question

I was given task to build a client server application, using any technology I want. Here are the requirements

1. To simplify the process the Server would have an in memory stock list and there would be a random data generator to update the stock data
2. Each client will have his own stock list
3. Each client can add stocks to its list
4. To simplify the process each client will use polling to get the stock prices, but the server needs to return only the changed stocks and no the whole list.
5. There will be an option to add new stocks to the repository.

I am focusing more on the server side here and for now the client is a console app. each client generates a token (guid) and sends it to the server as his ID. I used Nancy http server as the back end.

I would like you to please comment about the correctness of my implementation as if it was a code review for your team. OOP design, efficient and safe server implementation

I would appreciate any comments or questions.

1.Server Project

ServerModule.cs

namespace StocksApp
{ 

    public class ServerModule : Nancy.NancyModule
    {

        public ServerModule()
        {
            Post["/User/{id}"] = parameters => CreateUser(parameters);
            Get["/UserShares/{id}"] = parameters => GetUserShares(parameters);
            Post["/AddShareToUser/{id}/{share}"] = parameters => RegisterUserToShare(parameters);
            Post["/AddShareToRepository/{id}"] = parameters => AddShareToRepository(parameters);
        }

        private dynamic AddShareToRepository(dynamic parameters)
        {
            string id = parameters["id"].ToString();
            if (!string.IsNullOrEmpty(id))
            {
                if (!RepositoriesFactory.StocksRepository.AddStock(id))
                {
                    return JsonConvert.SerializeObject(new SimpleResponse() { IsSuccess = false });
                }
                return JsonConvert.SerializeObject(new SimpleResponse() { IsSuccess = true });
            }
            return JsonConvert.SerializeObject(new SimpleResponse
            {
                IsSuccess = false,
                Message = "Id null"
            });
        }

        private static dynamic RegisterUserToShare(dynamic parameters)
        {
            try
            {
                string id = parameters["id"].ToString();
                if (!string.IsNullOrEmpty(id))
                {
                    User user = RepositoriesFactory.UsersRepository.GetUser(id);
                    if (user.RegisterToStockUpdated(parameters["share"]))
                    {
                        return JsonConvert.SerializeObject(new SimpleResponse() { IsSuccess = true });
                    }
                    return JsonConvert.SerializeObject(new SimpleResponse() { IsSuccess = false, Message = "Could not register user" });
                }
                return JsonConvert.SerializeObject(new SimpleResponse
                {
                    IsSuccess = false,
                    Message = "Id null"
                });
            }
            catch (Exception e)
            {
                return JsonConvert.SerializeObject(new SimpleResponse
                {
                    IsSuccess = false,
                    Message = e.ToString()
                });
            }
        }

        private static dynamic GetUserShares(dynamic parameters)
        {
            try
            {
                string id = parameters["id"].ToString();
                if (!string.IsNullOrEmpty(id))
                {
                    User user = RepositoriesFactory.UsersRepository.GetUser(id);
                    string jsonData =
                        JsonConvert.SerializeObject(new StockListResponse
                        {
                            Stocks = user.GetUpdatedStocks(),
                            IsSuccess = true
                        });
                    return jsonData;
                }
                return JsonConvert.SerializeObject(new StockListResponse
                {
                    IsSuccess = false,
                    Message = "Id null"
                });
            }
            catch (Exception e)
            {
                return JsonConvert.SerializeObject(new StockListResponse
                {
                    IsSuccess = false,
                    Message = e.ToString()
                });
            }
        }

        private static dynamic CreateUser(dynamic parameters)
        {
            try
            {
                string id = parameters["id"].ToString();
                if (!string.IsNullOrEmpty(id))
                {
                    RepositoriesFactory.UsersRepository.AddUser(id);
                    return JsonConvert.SerializeObject(new SimpleResponse { IsSuccess = true });
                }
                return JsonConvert.SerializeObject(new SimpleResponse
                {
                    IsSuccess = false,
                    Message = "Id null"
                });
            }
            catch (Exception e)
            {
                return JsonConvert.SerializeObject(new SimpleResponse
                {
                    IsSuccess = false,
                    Message = e.ToString()
                });
            }
        }
    }
}

RepositoriesFactory.cs

namespace StocksApp
{
    public static class RepositoriesFactory
    {
        public static IUsersRepository UsersRepository { get;}
        public static IStocksRepository StocksRepository{ get; }

        static RepositoriesFactory()
        {
            StocksRepository = new StocksRepository();
            UsersRepository = new UsersRepository(StocksRepository);
        }
    }
}

IStocksRepository

namespace StocksApp.Repositories
{
    public interface IStocksRepository
    {
        void StartUpdats();
        IEnumerable<string> StockList { get; }

        Stock GetStock(string stockID);
        bool AddStock(string id);
    }
}

StocksRepository.cs

namespace StocksApp.Repositories
{
    public class StocksRepository : IStocksRepository
    {
        public ConcurrentDictionary<string, Stock> Stocks { get; set; }

        public Stock GetStock(string stockID)
        {
            if (Stocks.ContainsKey(stockID))
            {
                return Stocks[stockID];
            }
            return null;
        }
        public IEnumerable<string> StockList => Stocks.Keys;

        public StocksRepository()
        {
            Stocks = new ConcurrentDictionary<string, Stock>(
            new Dictionary<string, Stock> {
                {"INTC", new Stock("INTC") },
                {"GOOG", new Stock("GOOG") },
                {"MSC", new Stock("MSC") },
                {"AMD", new Stock("AMD") },
                {"AAPL", new Stock("AAPL") }}
            );
        }
        public bool AddStock(string id)
        {
            return Stocks.TryAdd(id, new Stock(id));
        }

        public void StartUpdats()
        {
            Random r = new Random();
            Task.Run(() =>
            {
                while (true)
                {
                    int index = r.Next(0, Stocks.Keys.Count);
                    double value = Math.Round(r.NextDouble() * 50, 2);
                    string stockName = StockList.ElementAt(index);
                    Console.WriteLine($"{stockName} was updated to {value}");
                    Stocks[stockName].UpdateValue(value);
                    Thread.Sleep(500);
                }

            });
        }


    }
}

IUsersRepository.cs

namespace StocksApp.Repositories
{
    public interface IUsersRepository
    {
        void AddUser(string id);
        User GetUser(string id);
        void RegisterForUpdated(string userID, string stockID);
    }
}

UsersRepository.cs

namespace StocksApp.Repositories
{
    public class UsersRepository : IUsersRepository
    {
        private readonly IStocksRepository _stockRepository;
        private Dictionary<string, User> _users = new Dictionary<string, User>();

        public UsersRepository(IStocksRepository stockRepository)
        {
            _stockRepository = stockRepository;
        }

        public void AddUser(string id)
        {
            _users[id] = new User("id",_stockRepository);
        }

        public User GetUser(string id)
        {
            if (_users.ContainsKey(id))
            {
                return _users[id];
            }
            return null;
        }

        public void RegisterForUpdated(string userID, string stockID)
        {
            if (_users.ContainsKey(userID))
            {
                _users[userID].RegisterToStockUpdated(stockID);
            }
        }
    }
}

User.cs

namespace StocksApp.Repositories
{
    public class User
    {
        private readonly IStocksRepository _stockRepository;
        private readonly ConcurrentDictionary<string,Stock> _registeredStocks = new ConcurrentDictionary<string,Stock>();
        private readonly IDictionary<string, double> _changedStocks = new Dictionary<string, double>();
        private readonly object _locker = new object();
        public string ID { get; }

        public User(string id, IStocksRepository stockRepository)
        {
            _stockRepository = stockRepository;

            ID = id;
        }

        public bool RegisterToStockUpdated(string stockID)
        {
            //so we dont register twice.
            if (_registeredStocks.ContainsKey(stockID))
            {
                return false;
            }
            Stock stock = _stockRepository.GetStock(stockID);
            if (stock != null)
            {
                stock.Updated += Stock_Updated;
                _registeredStocks.TryAdd(stockID,stock);
                return true;
            }
            return false;
        }

        private void Stock_Updated(Stock stock)
        {
            lock (_locker)
            {
                _changedStocks[stock.Name] = stock.Value;
            }
        }

        public IDictionary<string, double> GetUpdatedStocks()
        {
            IDictionary<string, double> returnValue;
            lock (_locker)
            {
                returnValue = new Dictionary<string, double>(_changedStocks);
                _changedStocks.Clear();
            }
            return returnValue;
        }
    }
}

there is also the bootstrapper for the console server application

Program.cs

namespace StocksApp
{
    class Program
    {
        private readonly IStocksRepository _stocksRepository;
        private readonly IUsersRepository _usersRepository;
        private string _url = "http://localhost";
        private int _port = 8080;
        private NancyHost _nancy;

        public Program(IStocksRepository stocksRepository, IUsersRepository usersRepository)
        {
            _stocksRepository = stocksRepository;
            _usersRepository = usersRepository;
            var uri = new Uri($"{_url}:{_port}/");
            var configuration = new HostConfiguration()
            {
                UrlReservations = new UrlReservations() { CreateAutomatically = true }
            };
            _nancy = new NancyHost(configuration,uri);
        }

        private void Start()
        {
            _nancy.Start();
            _stocksRepository.StartUpdats();
            Console.WriteLine($"Started listennig port {_port}");
            Console.ReadKey();
            _nancy.Stop();

        }

        static void Main(string[] args)
        {
            var p = new Program(RepositoriesFactory.StocksRepository, RepositoriesFactory.UsersRepository);
            p.Start();
        }
    }
}

2. SharedProject

Stock.cs

namespace StoksApp.Shared.Entities
{
    public class Stock
    {
        public string Name { get; set; }
        public double Value { get; set; }
        public event Action<Stock> Updated = delegate { };
        public Stock(string name)
        {
            Name = name;
        }

        public void UpdateValue(double value)
        {
            Value = value;
            Updated(this);
        }
    }
}

SimpleResponse.cs

namespace StoksApp.Shared.Entities
{
    public class SimpleResponse
    {
        public bool IsSuccess { get; set; }
        public string Message { get; set; }
    }
}

StockListResponse.cs

namespace StoksApp.Shared.Entities
{
    public class StockListResponse : SimpleResponse
    {
        public IDictionary<string, double> Stocks { get; set; }
    }
}

Answer 1

For me, the code looks well structured so far. Nancy is a great web framework, I like it :).

However, some remarks:

ServerModule.cs

Some code fragments like

return JsonConvert.SerializeObject(new SimpleResponse
{
    IsSuccess = false,
    Message = "Id null"
});

or

return JsonConvert.SerializeObject(new SimpleResponse
{
   IsSuccess = false,
   Message = e.ToString()
});

(and others) are redundant. I would extract such fragments to separate methods to increase readability.

---

Logging

You are just returning the the error message in case of exceptions. I would really suggest to log exceptions and any special behaviors on server side. Otherwise you are not able to analyze errors in production and analyzing errors in QA is also cumbersome without logging!

---

RepositoriesFactory.cs

It is good, that the repositories are already abstracted so they can be easily replaced with "real" implementations.

However, the RepositoryFactory has nothing to do with a factory - it is just a static container!

I would try to drop the class completely and use dependency injection instead. Nancy has its own DI-framework and supports also the usage of other frameworks.

---

StocksRepository.cs

Even if StartUpdats is just a dummy method that produces test data, I would suggest to start the task with option TaskCreationOptions.LongRunning; otherwise a threadpool thread (which used used for short running actions) will be blocked.

Further more, I would add a ContinueWith handler that logs the exception in case of failure. Otherwise exceptions are ignored because the task is not awaited.

---

User

It looks a little bit strange to me (violating the SRP), that the user gets the stock repository and registers event handlers to changes....

I would create another class that gets both repositories, registers itself for changes and updates user objects if anything changed.

Answer 2

ServerModule

I believe there is a lot of room for improving the handler methods:

• You're passing in dynamic parameters, but using it as a dictionary. Dynamic's strength is to make the code cleaner by writing parameters.id and letting the underlying system figure out how to extract the result. Not using it like this defeats the actual purpose of having dynamic.
• The methods shouldn't care where the parameters come from. Simply passing string id is much cleaner.
• Using dynamic as a return value is also overkill as the methods return strings in all cases.
• The methods shouldn't care how the reply gets serialized. Return the entity class and let the caller worry about in which format to serialize it (perhaps a client requests xml instead of json)
• IsNullOrWhitespace() might be a better fit than IsNullOrEmpty() when checking the id

• Exceptions could also be handled by the calling code. This will also let you reply with different HTTP status codes in case of issues.

  private SimpleResponse AddShareToRepository(string id)
  {
      if (string.IsNullOrWhitespace(id))
      {
          return new SimpleResponse
          {
              IsSuccess = false,
              Message = "Id null"
          };
      }
      return new SimpleResponse() { IsSuccess = RepositoriesFactory.StocksRepository.AddStock(id) };
  }
  

StocksRepository

Stocks.ContainsKey(stockID) followed by return Stocks[stockID] is not thread safe as the collection might change in between the calls. Use ConcurrentDictionary<TKey, TValue>.TryGetValue() for that.

Populating the dictionary with initial values contains duplicate names which can be prone to errors. This can be avoided by writing the ids only once: new [] { "INTC", "GOOG", "MSC", "AMD", "AAPL" }.ToDictionary(id => id, id => new Stock(id))

StartUpdats which keeps a thread running that performs updates should not be part of the repository. The repository should only provide a way of updating the value of a stock. Keeping a thread running that generates random values for updates should be the responsibility of another class.

UsersRepository

Like the previous repository, UsersRepository should also use a ConcurrentDictionary or some locking.

User

GetUpdatedStocks can be implemented without locking and copying the dictionary content. An atomic switch between _changedStocks and an empty dictionary should be enough.

public IDictionary<string, double> GetUpdatedStocks()
{
    return (IDictionary<string, double>)Interlocked.Exchange(ref _changedStocks, new Dictionary<string, double>());
}