3

I want ask experienced software developers and architects about this domain class which I found one day in some code.

public class User : Entity
{
    public virtual string Firstname { get; set; }
    public virtual string Lastname { get; set; }
    public User(string firstName, string lastName, IUserRepository userRepository, IUserService userService)
    {
        Firstname = firstName;
        Lastname = lastName
        userRepository.Save(this);
        userService.DoSomeStaff(this);
    }
}

What you think about SOLID principles in particular regarding the constructor? I think that it breaks at least the single responsibility principle, because:

  1. the constructor initializes the object, and
  2. saves the object to the DB using a repository, and
  3. does some stuff using a service.

Maybe there is some project architecture that recommends this code style?

Here are the definitions for the involved repository and service:

public class UserRepository : IUserRepository
{
    // some code of repository pattern with Nhibernate
    public void Save(User user)
    {
        //here Session is Nhibernate.ISession
        Session.SaveOrUpdate(user); 
    }
}

public class UserService : IUserService
{
    private readonly ISomeName1Repository _someName1Repository;
    private readonly ISomeName2Repository _someName2Repository;
    public UserService(ISomeName1Repository someName1Repository, ISomeName2Repository someName2Repository)
    {
        _someName1Repository = someName1Repository;
        _someName2Repository = someName2Repository;
    }

    public void DoSomeStaff(User user)
    {
        var foo = _someName1Repository.DoSomeStaff();
        // some work here with user
    }
}
  • 7
    It's unclear from the code whether or not this constitutes a "single responsibility." The constructor is doing too much, however. A constructor's sole purpose in life is to ensure the creation of a valid object, not to perform actual work. – Robert Harvey Nov 16 '17 at 16:18
  • 1
    Internet is overwhelmed with advices that constructors should not do any work except validation. Here is an example: yegor256.com/2015/05/07/ctors-must-be-code-free.html – Zapadlo Nov 16 '17 at 16:18
  • @Zapadlo Should it even be doing validation? – TheCatWhisperer Nov 16 '17 at 16:19
  • 3
    @TheCatWhisperer: It should throw an exception if the parameters provided are insufficient to ensure a valid object. – Robert Harvey Nov 16 '17 at 16:19
  • 1
    @Zapadlo If you saw the original code you will be surprised twice. – Stanislav Machel Nov 16 '17 at 16:54
3

What you think about SOLID principles

Well, at least the constructor uses dependency injection, so the D from SOLID is applied here, I guess? ;-) But I assume your question is about the SRP, not about SOLID in general.

So to be serious, this design looks to me like someone wanted to have a "callback" or "hook" to be called whenever a User object is created (probably with the special intention to make sure the object exists also in a repository or another service is informed about the objects creation).

Actually, I think it is debatable if this really violates the SRP, (since it will be easy to nullify a call like userRepository.Save by passing a dummy repo). For that reason, the two additional parameters do not prohibit unit testing or reuse of that class in a context where the repo and/or service is not needed (which would be a clear sign of violating the SRP).

However, I think this violates the more fundamental KISS principle. The injected interfaces make the calling code more complicated. We don't see the whole surrounding code, but IMHO it is (almost) always possible to keep the domain objects completely clean of any callbacks or service calls, and let the surrounding environment handle these things. To my experience, this will lead to a system which is overall simpler, easier to understand and easier to maintain.

1

Constructors are a grey area here. I wouldn't get caught up in the correctness it implies for the surrounding object. Same goes for static initializers and static factory methods. Focus on the instance methods of the object instead.

If you are worried about bloat seeping into the initializers then move that code out e.g. into a dependency injection framework.

  • Given the code in the question I'd say there is no gray area here. That constructor leaves the SRP shattered on the bathroom floor in a puddle of it's own tears. – Greg Burghardt Nov 17 '17 at 1:41
  • Do you drink dependency injection every night? – Frank Hileman Nov 17 '17 at 1:42
1

The point of SRP is about not implementing more than one responsibility in the same class. Your example class itself is only a container for first and last name, it has no more data and it has no more logic/behavior. It may call on other objects to perform different tasks but those tasks are not implemented in the User class so from an SRP perspective this is perfectly fine.

  • Do you really think that domain class constructor can not only initialize object, but also call repository methods and some services? – Stanislav Machel Nov 16 '17 at 20:20
  • @Stanislav That is a different question which I did not answer. – Martin Maat Nov 16 '17 at 22:07
  • I think it is debatable if the calls to the repository methods and services could be interpreted as "logic" or behaviour. Nevertheless, +1 for pointing out this possible interpretation. – Doc Brown Nov 18 '17 at 13:45

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.