0

I understand interface segregation and single responsibility principles guide against a class implementing methods or property it doesn't need. However, for the scenario described below where I have a User service with Login and Register methods, my Login Controller needs access to a single method (the login method). I have tried to hide the Register method by injecting a UserService object exposed through the ILogin interface.

Interfaces

public interface ILogin
{
   Task Login();
}
public interface IRegister
{
   Task Register();
}

and the UserService class;

public class UserService: IRegister, ILogin
{
   public Task Login()
   {
      //implementation logic here
   } 
   public Task Register()
   {
      //implementation logic here
   } 
}

the Login Controller client

public class LoginController : Controller
{
   private ILogin login;
   public Login Controller(ILogin login)
   {
      this.login = login;
   }
   [HttpPost]
   public IActionResult Login()
   {
      //implementation logic here for login.post
   } 
}

I would like to know if this implementation breaks any design principle or if it is an anti-pattern.

Mark Rotteveel
  • 100,966
  • 191
  • 140
  • 197
upsidedownwf
  • 134
  • 1
  • 3
  • 12
  • Then you'll need to register both services with DI explicitly. `services.AddScoped(); ....`. However is `User` a service? or is it data? – Jeremy Lakeman Apr 29 '22 at 03:42
  • 1
    It's really unclear what the problem is with your code. Looks like it should compile and work fine? – zaitsman Apr 29 '22 at 04:30
  • @zaitsman I know it would compile and work fine. My question is have I broken any design principle with this implementation or is it an antipattern? – upsidedownwf Apr 29 '22 at 11:10
  • @JeremyLakeman user is a service class. – upsidedownwf Apr 29 '22 at 11:11
  • 1
    @OlamideJon-Ajumobi I think you're overthinking it. Design principles make sense when you get to a 'code smell'. Like thousands of lines in a class. A method with tens of lines. That kind of stuff. With what you're doing it's a-okay, my issue would be with a naming convention you have. E.g. a `User` is an entity, a model. Not a `service`. So I would have a `IUserService` implemented by `UserService`. And if you want to really break down parts of your code look at CQRS (and/or Mediatr). – zaitsman Apr 29 '22 at 11:12
  • @zaitsman Userservice is a more befitting name. I would update my question – upsidedownwf Apr 29 '22 at 11:15
  • Looks fine to me. Maybe they should even be to different classes. – Magnus Apr 29 '22 at 11:26
  • 1
    "does this break _any_ design principle" is way too broad a question, and an opinion-based one at that. Do you have an _actual_, _practical_ problem with this code? If not, then it's off-topic to ask about it here (consider another site like Code Review or Software Engineering for open-ended/opinion-based questions for improvement or review). – TylerH Apr 29 '22 at 18:08

1 Answers1

2

If you want to use just one method from your service in Controller, then it is okay to use interface ILogin.

ISP is how interface can be consumed. It means don’t depend on more than you need. That is there is no need LoginController to consume the following methods like Register().

ISP's goal is to hide unnecessary information. Why? To minimize coupling and dependency.

Read more about ISP here

StepUp
  • 36,391
  • 15
  • 88
  • 148