11

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?

  • 6
    Just so it's said...you have not passed anything by reference here. Passing references by value is not at all the same thing. – cHao May 3 '18 at 22:00
  • 4
    @cHao: That's a distinction without a difference. The behavior of the code is the same regardless. – Robert Harvey May 3 '18 at 22:22
  • 2
    @JDDavis: Fundamentally, the only real difference between your two examples is that you've given a meaningful name to the set user name and set password actions. – Robert Harvey May 3 '18 at 22:24
  • 1
    All your calls are by reference here. You have to use a value type in C# to pass by value. – Frank Hileman May 3 '18 at 22:46
  • 2
    @FrankHileman: All the calls are by value here. That's the default in C#. Passing a reference and passing by reference are different beasts, and the distinction does matter. If user were passed by reference, the code could yank it out of the caller's hands and replace it by simply saying, say, user = null;. – cHao May 3 '18 at 22:50
10

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.

  • 2
    A method with a name like InsertUser is likely to have the side effect of inserting. I am not sure what "icky" means in this context. As for using a "user" that hasn't been added, you seem to have missed the point. If it hasn't been added, it's not a user, just a request to create a user. You're free to work with the request, of course. – John Wu May 3 '18 at 22:54
  • @candiedorange Those aren't side effects they the desired effects of calling the method. If you don't want to immediately add, you create another method that satisfies the use case. But that's not the use case passed in the question so is say that concern is not important. – Andy May 3 '18 at 22:57
  • @candiedorange If the coupling makes the client code easier I'd say that's fine. What devs or pos might thinkin the future is irrelevant. If that time comes, you change the code. Nothing is more wasteful than trying to build code that can handle any possible future use case. – Andy May 3 '18 at 23:06
  • 5
    @CandiedOrange I suggest you try not to tightly couple the precisely defined type system of the implementation with the conceptual, plain-English entities of the business. A business concept of "user" can certainly be implemented in two classes, if not more, to represent functionally significant variants. A user who is not persisted is not guaranteed a unique user name, has no primary key to which transactions could be tied, and can't even sign on, so I'd say it comprises a significant variant. To a layman, of course, both the EnrollRequest and the User are "users," in their vague language. – John Wu May 4 '18 at 0:03
4

Side effects are ok as long as they don't come unexpected. So there is nothing wrong in general when a repository has a method accepting a user and changes the user's internal state. But IMHO a method name like InsertUser does not clearly communicate this, and that is what makes it error prone. When using your repo, I would expect a call like

 repo.InsertUser(user);

to change the repositories internal state, not the user object's state. This problem exists in both of your implementations, how InsertUser does this internally is completely irrelevant.

To solve this, you could either

  • separate initialization from the inserting (so the caller of InsertUser needs to provide a fully initialized User object, or,

  • build the initialization into the construction process of the user object (as suggested by some of the other answers), or

  • simply try to find a better name for the method which expresses more clearly what it does.

So choose a method name like PrepareAndInsertUser, OrchestrateUserInsertion, InsertUserWithNewNameAndPassword or whatever you prefer to make the side effect more clearer.

Of course, such a long method name indicates that the method maybe is doing "too much" (SRP violation), but sometimes you don't want or cannot easily fix this, and this is the least intrusive, pragmatic solution.

3

I'm looking at the two options you've chosen, and I have to say that I by far prefer the old method rather than the proposed new method. There's a few reasons for it even though they fundamentally do the same thing.

In either case you are setting the user.UserName and user.Password. I have reservations about the password item, but those reservations aren't germane to the topic at hand.

Implications of modifying reference objects

  • It makes concurrent programming more difficult--but not all applications are multithreaded
  • Those modifications can be surprising, particularly if nothing about the method suggests that it would happen
  • Those surprises can make maintenance more difficult

Old Method vs. New Method

The old method made things easier to test:

  • GenerateUserName() is independently testable. You can write tests against that method and make sure the names are generated correctly
  • If the name requires information from the user object, then you can change the signature to GenerateUserName(User user) and maintain that testability

The new method hides the mutations:

  • You don't know that the User object is changing until you go 2 layers deep
  • The changes to the User object are more surprising in that case
  • SetUserName() does more than set a user name. That's not truth in advertising which makes it harder for new developers to discover how things work in your application
1

I fully agree with John Wu's answer. His suggestion is a good one. But it slightly misses your direct question.

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

Not inherently.

You can't take this too far, as you'll run into unexpected behavior. E.g. PrintName(myPerson) should not be changing the person object, since the method implies it's only interested in reading the existing values. But that is a different argument than for your case, since SetUsername(user) strongly implies that it's going to set values.


This is actually an approach I often use for unit/integration tests, where I create a method specifically to alter an object in order to set its values to a particular situation I want to test.

For example:

var myContract = CreateEmptyContract();

ArrrangeContractDeletedStatus(myContract);

I explicitly expect the ArrrangeContractDeletedStatus method to change the state of the myContract object.

The main benefit is that the method allows me to test contract deletion with different initial contracts; e.g. a contract that has a long status history, or one that has no previous status history, a contract that has an intentionally erroneous status history, a contract that my test user is not allowed to delete.

If I had merged CreateEmptyContract and ArrrangeContractDeletedStatus into a single method; I would have to create multiple variants of this method for every different contract I'd want to test in a deleted state.

And while I could do something like:

myContract = ArrrangeContractDeletedStatus(myContract);

This is either redundant (since I'm changing the object anyway), or I'm now forcing myself to make a deep clone of the myContract object; which is excessively difficult if you want to cover every case (imagine if I want several levels' worth of navigational properties. Do I need to clone all of them? Only the top level entity? ... So many questions, so many implicit expectations)

Changing the object is the easiest way to get what I want without having to do more work just to avoid not changing the object as a matter of principle.


So the direct answer to your question is that it's not inherently bad practice, as long as you don't obfuscate that the method is liable to change the passed object. For your current situation, that is made abundantly clear through the method name.

0

I find the old as well as the new problematic.

Old way:

1) InsertUser(User user)

When reading this method name popping up in my IDE, the first thing, which comes to my mind is

Where is the user inserted into?

The method name should read AddUserToContext. At least, this is, what the method does in the end.

2) InsertUser(User user)

This violates clearly the principle of the least surprise. Say, I did my work so far and created a freshly instance of a User and gave him a name and set a password, I would be struck by surprise:

a) this does not only insert a user into something

b) it also does also subvert my intention to insert the user as is; the name and the password were overridden.

c) does this indicate, that it is also a violation of the single responsibility principle.

New way:

1) InsertUser(User user)

Still violation of SRP and principle of the least surprise

2) SetUsername(User user)

Question

Set username? To what?

Better: SetRandomName or something what reflects the intention.

3) SetPassword(User user)

Question

Set password? To what?

Better: SetRandomPassword or something what reflects the intention.


Suggestion:

What I would prefer reading would be something like:

public User GenerateRandomUser(UserFactory Uf){
    User u = Uf.GetUser();
    u.Name = GenerateRandomUserName();
    u.Password = GenerateRandomUserPassword();
    return u;
}

...

public void AddUserToContext(User u){
    this.context.Users.Add(u);
}

Regarding your initial question:

Is modifying an object passed by reference a bad practice?

No. It's more a matter of taste.

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.