C# – Is modifying an object passed by reference a bad practice

ccoding-styledesign

In the past, I've typically done most of my manipulation of an object within the primary method it's being created/updated, but I've found myself taking a different approach lately, and I'm curious if it's a bad practice.

Here's an example. Let's say I have a repository that accepts a User entity, but prior to inserting the entity, we call some methods to make sure all its fields are set to what we're wanting. Now, rather than calling methods and setting the field values from within the Insert method, I call a series of preparation methods that shape the object prior to its insertion.

Old Method:

public void InsertUser(User user) {
    user.Username = GenerateUsername(user);
    user.Password = GeneratePassword(user);

    context.Users.Add(user);
}

New Methods:

public void InsertUser(User user) {
    SetUsername(user);
    SetPassword(user);

    context.Users.Add(user);
}

private void SetUsername(User user) {
    var username = "random business logic";

    user.Username = username;
}

private void SetPassword(User user) {
    var password = "more business logic";

    user.Password = password;
}

Basically, is the practice of setting a property's value from another method a bad practice?

Best Answer

The issue here is that a User can actually contain two different things:

  1. A complete User entity, which can be passed to your data store.

  2. The set of data elements required from the caller in order to begin the process of creating a User entity. The system must add a user name and password before it is truly a valid User as in #1 above.

This comprises an undocumented nuance to your object model that isn't expressed in your type system at all. You just need to "know" it as a developer. That aint great, and it leads to weird code patterns like the one you are encountering.

I'd suggest you need two entities, e.g. a User class and an EnrollRequest class. The latter can contain everything you need to know to create a User. It would look like your User class, but without the user name and password. Then you could do this:

public User InsertUser(EnrollRequest request) {
    var userName = GenerateUserName();
    var password = GeneratePassword();

    //You might want to replace this with a factory call, but "new" works here as an example
    var newUser = new User
    (
        request.Name, 
        request.Email, 
        userName, 
        password
    );
    context.Users.Add(user);
    return newUser;
}

The caller starts with only enrollment information and gets back the completed user after it is inserted. This way you avoid mutating any classes and you also have a type-safe distinction between a user who is inserted and one who is not.

Related Topic