43

Let's look at your code from an extensibility point of view: If Sheldon decides to add a new item to the game then you have to go to n classes to adjust the comparisons and winning verbs. I usually try to avoid such designs because whenever you require a developer to change stuff in n places when something new is added then he/she is bound to forget one ...


42

I coded keeping SOLID principle in mind and here is my code. What did I miss? Let's see... SRP - Single Responsibility Principle A class should have only one reason to change. Violated. The name EmployeeLeave suggests it's a class that just stores some data about an employee-leave but its API says something else - I'm a repository. So what is it? ...


31

Your Main method has too many responsibilities: it's your application's composition root, and it's executing the program's logic... if printing to Console can be considered logic. There's not enough meat in this project to even justify using DI/IoC: you have a service that nothing depends on. If you had, say, an Application class: public interface ...


17

public class EmployeeLeave : ILeaveRequest ... var leaveRequest = new EmployeeLeaveDetail(); This is confusing. Going by name, ILeaveRequest implies that the class implementing that interface is a "leave request" (I'm aware that classes don't have an "is a" relationship with interfaces). But then we see an actual leave request in the form of an ...


15

this sounded like IComparable<T> But it's not. The documentation of Compare() states that the relation has to be transitive: If A.CompareTo(B) returns a value x that is not equal to zero, and B.CompareTo(C) returns a value y of the same sign as x, then A.CompareTo(C) is required to return a value of the same sign as x and y. This isn't true in ...


14

catch (Exception) { throw; } Well this won't buy you anything but a few more lines of code. Just remove the try..catch at all. This construct just let the exception bubble up the call stack like it would if you wouldn't have the try..catch at all. public void ProcessLeaveRequest You don't validate all of the given method arguments. The reason ...


13

Style/Readability Unnecessary XML comments. For example: /// <summary> /// Gets the failure count. /// </summary> /// <value> /// The failure count. /// </value> public int FailCount { get { return _failCount; } } The summary and value not only repeat each other, but also repeat the property name. Statement in comments of what's ...


12

I would say your design is fine. In general I advise injecting IHandleViewModel<T> implementations directly into consumers, because this makes it easier to verify the object graph directly and makes it clearer what the consumer actually depends on. You should do this, unless you regularly inject multiple IHandleViewModel<T>s into the same ...


12

First of all, kudos for exercising OOP and DI! Coding against interfaces is going to make your life easier in so many wonderful ways! I agree, no problem is too simple to solve with SOLID principles, especially for learning purposes. Shifting from procedural to object-oriented code takes practice. The first key to proper OOP is abstraction. These are the ...


11

Putting this as an anwer, because it took a while to find it: I took your code, and put it through some tests..... I made the tests up based on the 'rules'. This is the sort of thing any interviewer will do: public static void main(String[] args) { String[] positive = {"12345abc", "1a2b3c4d", "1234567890ab", "1234a"}; String[] negative = {"", "1", "...


10

Your question isn't crystal-clear about what it is exactly that your code is supposed to be doing. We have to infer the functionality from this block of code: public void Start() { List<Network> networks = context.Networks.ToList(); foreach (var network in networks) { // create new client...but also // new client gets ...


9

A lot of useful comments is here but no-one has commented about the usage of DateTime. EmployeeLeave DateTime.Now - employee.StartDate I encourage you to process all the dates as UTC and only convert them to local time when you need to display. Even if you are sue your application will be used in one time-zone you save yourself from a lot of hassle in ...


9

It would be better to give the Irequest interface an additional "Name" property that returns the value of requestType. Than you can iterate over all Irequest implementations and get the first with the corresponding name ;). Example: interface IRequest { void CallService(); string Name { get; } } class EnrolmentRequest : IRequest { public void ...


8

I've not used PostSharp in a number of years, but have often used the following pattern to get around life-cycle issues with DI on attributes. It's a little manual, so should be seen as the exception rather than the rule, but it's easy to follow. In your aspect, define a static factory method specifically designed to create your dependency . Define it as a ...


8

I chose an Evaluate factory method with a private constructor I think that GameResult.Evaluate() doesn't logically make much sense, this logic belongs to a method in Game, which means the constructor of GameResult would be public. Also, why does GameResult contain Selection player? Something like bool playerWon would be enough. Console.WriteLine("Rock-Paper-...


8

No. What you are doing is called The Service Locator Antipattern. And it's a bad thing. The basic rule of thumb: Don't inject the container!. The IoC container is supposed to be some sort of overseer, wiring your objects together at the bootstrap and configuration level. It isn't an object that's supposed to be injected into other objects, to create ...


8

Yes, you need a IOC container to maintain your instances. No, your Unit of Work should not create them. With an IOC Container, you centralize the control over the objects and their lifetime. This is extremely powerful. Never should you have an instantiation of unit of work, repositories, ... throughout your application code. You're not the first person ...


8

Do I need ninject when implementing DAL with generic repository and unit of work? That's not the question. Inversion of control, repository and unit of work are patterns. Ninject is an IoC container - a tool that resolves dependencies and controls instantiation and object lifetime. You need Ninject (excellent choice, but many other IoC containers are ...


8

I think this is pretty much done. You have great naming. No glaring bugs as far as I can tell. It's clear and concise. Very OOP, which is impressive given the language. Even the high level design seems pretty darn tight. (I know very little about dependency injection though. I could have missed something that would be obvious to someone else.) I just can't ...


8

The NewItem implementation in the CustomerGroupsRepository has something weird going on: Set NewItem = result.Mock(model, items) Why add a Mock method to the SqlResultRow type? The SqlResultRow worked fine before, it shouldn't have been modified to accomodate the IRepository implementations. In fact, the NewItem implementation could be written like this: ...


8

Generally speaking, unit tests should follow the "Arrange, Act, Assert"-pattern. Yours only have the "Arrange, Act"-part. You're not asserting anything (with the exception of the one where you're expecting an exception to be thrown). [TestMethod] public void IsMarriedAndLessThan90Days() { // Arrange EmployeeLeave leave = new EmployeeLeave(new ...


7

First: Don't create a base class with a virtual method which simply throws. This moves problem detection from compile time (abstract member not implemented won't compile) to run time (forgotten to override or accidentally called base throws) - usually undesirable. Virtual method says "You can override me if you want but you don't have to". Now your actual ...


7

The problem with building a good base class is that ideally, you should know exactly what kind of functionality will the derived classes need from the base class. If you think they'll need more than is actually necessary, you'll write more code unnecessarily and it will also make the base class harder to use. On the other hand, if you think the derived ...


7

We could perhaps do away with the bulk of the children class code by implementing a bit of logic in the base class. A starting point for discussion would be something along the lines of: public abstract class SelectionBase : IComparable<SelectionBase> { private readonly List<WinningPlay> _winsAgainst; protected SelectionBase(List<...


7

There's usually very little I can say about your code, but maybe if I just start going down line by line I'll find something. I'm hoping someone else who knows more about mvp and dependency-injection come along as well. Simple Form I don't see Types uses very often in vba. Normally when I do, what is really needed is a class. I don't think that's the case ...


7

public interface IUnitOfWork : IDisposable { IRepository<T> Repository<T>() where T : class; void Commit(); void Rollback(); } The goal of a unit of work is to abstract the disposable stuff that's implementing it. By making your interface extend IDisposable, you've made a leaky abstraction - now anyone (including your mocking ...


7

Why the marker interface? public interface IViewModel { // Marker interface } public interface IHandleViewModel<TViewModel> where TViewModel : IViewModel { Task<TViewModel> Handle(); } What you really want to do here, is ensure that TViewModel is a reference type - right? There's a generic type constraint specifically for that: public ...


7

"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 ...


7

Here be dragons :P This is a conceptual minefield. I have done this exact thing and can relate, you instinctively want to test a lifecycle. a start-to-finish stub implementation..... but don't. the golden rule of unit testing is never test an interface. test an implementation. I'd add a second rule personally, don't test module interaction. If you ...


6

Service Locator Alert!! Your SetupHub class has a hardwired dependency on the IoC container itself - this is not dependency injection. When DI is done right, you cannot tell whether you're using an IoC container or if you're injecting the dependencies by hand - because the only place in your entire program that needs the IoC container, is the composition ...


Only top voted, non community-wiki answers of a minimum length are eligible