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?
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.
var myContract = CreateEmptyContract();
I explicitly expect the
ArrrangeContractDeletedStatus method to change the state of the
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
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.