SOLID principles in this class

Question

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
    }
}

Answer 1

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.

Answer 2

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.

Answer 3

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.