Designing a Class to take whole classes as parameters rather than individual properties

Question

Let's say, for example, you have an application with a widely shared class called User. This class exposes all information about the user, their Id, name, levels of access to each module, timezone etc.

The user data are obviously widely referenced throughout the system, but for whatever reason, the system is set up so that instead of passing this user object into classes that depend on it, we're just passing in individual properties from it.

A class that requires the user id, will simply require the GUID userId as a parameter, sometimes we might need the username as well, so that is passed in as a separate parameter. In some cases, this is passed to individual methods, so the values are not held at the class level at all.

Every single time I need access to a different piece of information from the User class, I have to make changes by adding parameters and where adding a new overload is not appropriate, I have to change every reference to the method or class constructor as well.

The user is just one example. This is widely practiced in our code.

Am I right in thinking this is a violation of the Open/Closed principle? Not just the act of changing existing classes, but setting them up in the first place so that widespread changes are very likely to be required in the future?

If we just passed in the User object, I could make a small change to the class I am working with. If I have to add a parameter, I might have to make dozens of changes to references to the class.

Are any other principles broken by this practice? Dependency inversion perhaps? Although we're not referencing an abstraction, there is only one kind of user, so there isn't any real need to have a User interface.

Are there other, non-SOLID principles being violated, such as basic defensive programming principles?

Should my constructor look like this:

MyConstructor(GUID userid, String username)

Or this:

MyConstructor(User theUser)

Post Edit:

It has been suggested that the question is answered in "Pass ID or Object?". This does not answer the question of how the decision to go either way affects an attempt to follow the SOLID principles, which is at the core of this question.

@gnat: This is definitely not a duplicate. The possible duplicate is about method chaining to reach deep into an object hierarchy. This question doesn't appear to be asking about that at all. — Greg Burghardt, Jun 26 '18 at 11:15
The second form is often used when the number of parameters being passed has become unwieldy. — Robert Harvey, Jun 26 '18 at 13:19
The one thing I don't like about the first signature is that there's no guarantee that the userId and username are actually originating from the same user. It's a potential bug that's avoided by passing around User everywhere. But the decision really depends on what the called methods are doing with the arguments. — 17 of 26, Jun 26 '18 at 13:26
The word “parse” makes no sense in the context in which you’re using it. Did you mean “pass” instead? — Konrad Rudolph, Jun 26 '18 at 14:21
Both are valid. The cost of creating a valid instance of User may not be worth it in some cases where you just need the id and username. The cost of creating a valid instance of User and passing it around may be worth it. These costs may be maintenance, performance, readability, etc. — bitsoflogic, Jun 26 '18 at 15:32
What about the I in SOLID? MyConstructor basically says now "I need a Guid and a string". So why not have an interface providing a Guid and a string, let User implement that interface and let MyConstructor depend on an instance implementing that interface? And if the needs of MyConstructor change, change the interface. -- It helped me greatly to think of interfaces to "belong" to the consumer rather than the provider. So think "as a consumer I need something that does this and that" instead of "as a provider I can do this and that". — Corak, Jun 27 '18 at 07:37
It's hard to say with any certainty without a real concrete example of a class with MyConstructor but to me this looks like the problem might be higher level - i.e. you can avoid this problem altogether with better encapsulation. With good encapsulation and adherence to the "tell, don't ask" principle, you shouldn't really need to pass property values of a User to a constructor. In practice, if that isn't feasible, everything else is going to be a compromise. — Ant P, Jun 27 '18 at 08:21
Pass ID or Object? does not in any way address how the decision to go either way can be judged to abide by or break software design principles, which is at the core of this question. — Jimbo, Jun 27 '18 at 09:25

score 31 · Answer 1 · answered Jun 26 '18 at 11:38

31

There is absolutely nothing wrong with passing an entire User object as a parameter. In fact, it might help clarify your code, and make it more obvious to programmers what a method takes if the method signature requires a User.

Passing simple data types is nice, until they mean something other than what they are. Consider this example:

public class Foo
{
    public void Bar(int userId)
    {
        // ...
    }
}

And an example usage:

var user = blogPostRepository.Find(32);
var foo = new Foo();

foo.Bar(user.Id);

Can you spot the defect? The compiler can't. The "user Id" being passed is just an integer. We name the variable user but initialize its value from the blogPostRepository object, that presumably returns BlogPost objects, not User objects - yet the code compiles and you end up with a wonky runtime error.

Now consider this altered example:

public class Foo
{
    public void Bar(User user)
    {
        // ...
    }
}

Maybe the Bar method only uses the "user Id" but the method signature requires a User object. Now let's go back to the same example usage as before, but amend it to pass the entire "user" in:

var user = blogPostRepository.Find(32);
var foo = new Foo();

foo.Bar(user);

Now we have a compiler error. The blogPostRepository.Find method returns a BlogPost object, which we cleverly call "user". Then we pass this "user" to the Bar method and promptly get a compiler error, because we can't pass a BlogPost to a method that accepts a User.

The Type System of the language is being leveraged to write correct code quicker, and identify defects at compile time, rather than run time.

Really, having to refactor lots of code because user information changes is merely a symptom of other problems. By passing an entire User object you gain the benefits above, in addition to the benefits of not having to refactor all method signatures that accept user information when something about the User class changes.

answered Jun 26 '18 at 11:38

Greg Burghardt

37,083

6

I'd say your reasoning by itself actually points towards passing fields but having the fields be trivial wrappers around the real value. In this example, a User has a field of type UserID and a UserID has a single integer-valued field. Now the declaration of Bar tells you immediately that Bar doesn't use all the information about the User, only their ID, but you still can't make any silly mistakes like passing an integer that didn't come from a UserID into Bar. – Ian Jun 26 '18 at 14:10
(Cont.) Of course this kind of programming style is quite tedious, especially in a language that doesn't have nice syntactic support for it (Haskell is nice for this style for example, since you can just match on "UserID id"). – Ian Jun 26 '18 at 14:12
5

@Ian: I think wrapping an Id in its own type skates around the original issue brought up by the OP, which is structural changes to the User class make it necessary to refactor many method signatures. Passing the entire User object solves this problem. – Greg Burghardt Jun 26 '18 at 15:33
@Ian: Although to be honest, even working in C# I've been very tempted to wrap Ids and the sort in a Struct just to give a little more clarity. – Greg Burghardt Jun 26 '18 at 15:34
Minor side point -- if User is stored on the stack, and thus would have to be copied in all of these places, there's nothing wrong with passing a pointer around in its place. You get all the same advantages, and if your User class is large, you also don't end up copying unnecessary data all the time. That's something you'd want to profile to see if it makes a difference, though, unless your User class is ridiculously big. – Jun 26 '18 at 21:35
@NickHartley: In the context of Java or C# those objects would be allocated on the heap and garbage collected. Surely something to consider with the likes of C++, though. – Greg Burghardt Jun 26 '18 at 22:42
1

"there's nothing wrong with passing a pointer around in its place." Or a reference, to avoid all of the issues with pointers that you could run into. – Yay295 Jun 26 '18 at 23:57
What if I were to add that the class I am working with is serialized and sent to the client side regularly. So there is an incentive to keep it light. – Jimbo Jun 27 '18 at 09:16

David Arno · Answer 2 · 2018-06-26T13:13:02.987

Am I right in thinking this is a violation of the Open/Closed principle?

No, it isn't a violation of that principle. That principle is concerned with not changing User in ways that affect other parts of the code that use it. Your changes to User could be such a violation, but it's unrelated.

Are any other principles broken by this practice? Dependency inversion perahaps?

No. What you describe - only injecting the required parts of a user object into each method - is the opposite: it's pure dependency inversion.

Are there other, non-SOLID principles being violated, such as basic defensive programming principles?

No. This approach is a perfectly valid way of coding. It isn't violating such principles.

But dependency inversion is only a principle; it's not an unbreakable law. And pure DI can add complexity to the system. If you find that only injecting the needed user values into methods, rather than passing the whole user object into either the method or constructor, creates problems, then don't do it that way. It's all about getting a balance between principles and pragmatism.

To address your comment:

There are problems with having to unnecessarily parse a new value down five levels of the chain and then change all of the references to all five of those existing methods...

Part of the issue here is that you clearly don't like this approach, as per the "unnecessarily [pass]..." comment. And that's fair enough; there's no right answer here. If you find it burdensome then don't do it that way.

However, with respect to the open/closed principle, if you follow that strictly then "...change all of the references to all five of those existing methods..." is an indication that those methods were modified, when they should be closed to modification. In reality though, the open/closed principle makes good sense for public APIs, but doesn't make much sense for the internals of an app.

...but surely a plan to abide by that principle as far as it is practicable to do so would include strategies to reduce the need for future change?

But then you wander into YAGNI territory and it still would be orthogonal to the principle. If you have method Foo that takes a user name and then you want Foo to take a a date of birth too, following the principle, you add a new method; Foo remains unchanged. Again that's good practice for public APIs, but it's a nonsense for internal code.

As previously mentioned, it's about balance and common sense for any given situation. If those parameters often change, then yes, use User directly. It'll save you from the large-scale changes you describe. But if they don't often change, then passing in only what's needed is a good approach too.

There are problems with having to unnecessarily parse a new value down five levels of the chain and then change all of the references to all five of those existing methods.
Why would the Open/Closed principle apply only to the User class and not also to the class I am currently editing, which is also consumed by other classes? I'm aware that the principle is specifically about avoiding change, but surely a plan to abide by that principle as far as it is practicable to do so would include strategies to reduce the need for future change? — Jimbo, Jun 26 '18 at 12:19
@Jimbo, I've updated my answer to try and address your comment. — David Arno, Jun 26 '18 at 13:13
I appreciate your contribution. BTW. Not even Robert C Martin accepts the Open/Closed principle has a hard rule. It's a rule of thumb that will inevitably be broken. Applying the principle is an exercise in attempting to abide it as much as is practicable. Which is why I used the word "practicable" previously. — Jimbo, Jun 26 '18 at 13:46
It's not dependency inversion to pass the parameters of User rather than User itself. — James Ellis-Jones, Jun 27 '18 at 10:06
@JamesEllis-Jones, Dependency invertion flips the dependencies from "ask", to "tell". If you pass in a User instance and then query that object to get a parameter, then you are only partially inverting the dependencies; there's still some asking going on. True dependency inversion is 100% "tell, don't ask". But it comes at a complexity price. — David Arno, Jun 27 '18 at 10:09
@DavidAmo I'd certainly agree that this is similar to dependency inversion in that both things are techniques to limit the scope of the class i.e. restrict what it knows to the minimum, and that there's a complexity cost to doing this. Just in terms of how dependency inversion is usually defined, I'd still have to disagree as it involves replacing a concrete dependency with an abstract one. — James Ellis-Jones, Jun 27 '18 at 11:15

score 10 · Answer 3 · answered Jun 26 '18 at 14:34

10

Yes, changing an existing function is a violation of the Open/Closed Principle. You’re modifying something that should be closed to modification due to requirements change. A better design (to not change when requirements change) would be to pass in the User to things that should work on users.

But that might run afoul of the Interface Segregation Principle, since you could be passing along way more information than the function needs to do its work.

So, as with most things - it depends.

Using just a username let’s the function be more flexible, working with usernames regardless of where they come from and without the need to make a fully functioning User object. It provides resilience to change if you think that the source of data will change.

Using the whole User makes it clearer about the usage and makes a firmer contract with its callers. It provides resilience to change if you think that more of the User will be needed.

answered Jun 26 '18 at 14:34

Telastyn

109,398

+1 but I am not sure about your phrase "you could be passing along way more information". When you pass (User theuser) you pass the very minimum of information, a reference to one object. True that reference can be used to get more information, but it means the calling code doesn't have to get it. When you pass (GUID userid, string username) the called method can always call User.find(userid) to find the object's public interface, so you don't really hide anything. – dcorking Jun 26 '18 at 15:04
5

@dcorking, "When you pass (User theuser) you pass the very minimum of information, a reference to one object". You pass the maximum information related to that object: the entire object. "the called method can always call User.find(userid)...". In a well designed system, that would not be possible as the method in question would have no access to User.find(). In fact there shouldn't even be a User.find. Finding a user should never be the responsibility of User. – David Arno Jun 26 '18 at 15:11
2

@dcorking - enh. That you're passing a reference that happens to be small is technical coincidence. You're coupling the entire User to the function. Maybe that makes sense. But maybe the function should only care about the user's name - and passing along stuff like the User's join date, or address is improper. – Telastyn Jun 26 '18 at 15:21
@DavidArno perhaps that is key for a clear answer to OP. Whose responsibility should finding a User be? Is there a name for the design principle of separating the finder/factory from the class? – dcorking Jun 26 '18 at 16:05
1

@dcorking I would say that is one implication of the Single Responsibility Principle. Knowing where Users are stored and how to retrieve them by ID are separate responsibilities a User-class should not have. There may be a UserRepository or similar that deals with such things. – Hulk Jun 27 '18 at 10:32

Theo Lenndorff · Answer 4 · 2018-06-27T19:27:09.370

This design follows the Parameter Object Pattern. It solves problems which arise from having many parameters in the method signature.

Am I right in thinking this is a violation of the Open/Closed principle?

No. Applying this pattern enables the Open/close principle (OCP). For instance derivative classes of User can be provided as parameter which induce a different behavior in the consuming class.

Are any other principles broken by this practice?

It can happen. Let me explain on the basis of the SOLID principles.

The Single responsibility principle (SRP) can be violated if it has the design as you have explained:

This class exposes all information about the user, their Id, name, levels of access to each module, timezone etc.

The problem is with all information. If the User class has many properties, it becomes a huge Data Transfer Object which transports unrelated information from the perspective of the consuming classes. Example: From the perspective of a consuming class UserAuthentication the property User.Id and User.Name are relevant, but not User.Timezone.

The Interface segregation principle (ISP) is also violated with a similiar reasoning but adds another perspective. Example: Suppose a consuming class UserManagement requires the property User.Name to be split up to User.LastName and User.FirstName the class UserAuthentication must be also modified for this.

Fortunately ISP also gives you a possible way out of the problem: Usually such Parameter Objects or Data Transport Objects start small and grow by time. If this becomes unwieldy consider following approach: Introduce interfaces tailored to the needs of the consuming classes. Example: Introduce interfaces and let the User class derive from it:

class User : IUserAuthenticationInfo, IUserLocationInfo { ... }

Each interface should expose a subset of related properties of the User class needed for a consuming class to fullfill its operation. Look for clusters of properties. Try to reuse the interfaces. In the case of the consuming class UserAuthentication use IUserAuthenticationInfo instead of User. Then if possible break up the User class into multiple concrete classes using the interfaces as "stencil".

Once User gets complicated, there is a combinatorial explosion of possible subinterfaces for example, if User has just 3 properties, there are 7 possible combinations. Your proposal sounds nice but is unworkable. — user949300, Jun 27 '18 at 18:51

Dom · Answer 5 · 2018-06-27T12:47:27.160

When confronted with this issue in my own code, I have concluded that basic model classes/objects are the answer.

A common example would be the repository pattern. Often when querying a database via repositories, many of the methods in the repository take a lot of the same parameters.

My rules-of-thumb for repositories are:

Where more than one method takes the same 2 or more parameters, the parameters should be grouped together as a model object.
Where a method takes more than 2 parameters, the parameters should be grouped together as a model object.
Models can inherit from a common base, but only when it really makes sense (usually better to refactor later than start with inheritance in mind).

The problems with using models from other layers/areas don't become apparent until the project starts to become a little bit complex. It's only then that you find less code creates more work or more complications.

And yes, it's totally fine to have 2 different models with identical properties that serve different layers/purposes (ie. ViewModels vs POCOs).

score 2 · Answer 6 · answered Jun 29 '18 at 10:08

Let's just check the individual aspects of SOLID:

Single responsibility: possibly fiolated if people tend to pass around only sections of the class.
Open/closed: Irrelevant where sections of the class are passed around, only where the full object is passed around. (I think that's where the cognitive dissonance kicks in: you need to change faraway code but the class itself seems fine.)
Liskov substitution: Non-issue, we're not doing subclasses.
Dependency inversion (depend on abstractions, not concrete data). Yeah that's violated: People don't have abstractions, they take out concrete elements of the class and pass that around. I think that's the main issue here.

One thing that tends to confuse design instincts is that the class is essentially for global objects, and essentially read-only. In such a situation, violating abstractions does not hurt very much: Just reading data that isn't modified creates a pretty weak coupling; only when it becomes a huge pile the pain becomes noticeable.
To reinstate design instincts, just assume that the object isn't very global. What context would a function need if User object could be mutated anytime? What components of the object would likely be mutated together? These can be split out of User, whether as a referenced subobject or as an interface that exposes just a "slice" of related fields isn't that important.

Another principle: Look at the functions that use parts of User and see which fields (attributes) tend to go together. That's a good preliminary list of subobjects - you definitely need to think whether they actually belong together.

It's lot of work, and it's somewhat hard to do, and your code will become slightly less flexible because it will become slightly harder to identify the subobject (subinterface) that needs to be passed to a function, particularly if the subobjects have overlaps.

Splitting User up will actually get ugly if the subobjects overlap, then people will be confused about which one to choose if all the required fields are from the overlap. If you split up hierarchically (e.g. you have UserMarketSegment which, among other things, has UserLocation), people will be unsure what level the function they're writing is at: is it dealing with user data at the Location level or at the MarketSegment level? It doesn't exactly help that this can change over time, i.e. you're back at changing function signatures, sometimes across a whole call chain.

In other words: Unless you really know your domain and have a pretty clear idea of what module is dealing with what aspects of User, it's not really worth improving the program's structure.

score 2 · Answer 7 · answered Jul 02 '18 at 18:23

Here's something I've encountered from time to time:

A method takes an argument of type User (or Product or whatever) which has lots of properties, even though the method only uses a few of them.
For some reason, some part of the code needs to call that method even though it doesn't have a fully-populated User object. It creates an instance and initializes just the properties the method actually needs.
This happens a bunch of times.
After a while, when you encounter a method that has a User argument, you find yourself having to find calls to that method to find where the User comes from so that you know which properties are populated. Is it a "real" user with an email address, or was it just created to pass a user ID and some permissions?

If you create a User and only populate a few properties because those are the ones the method needs, then the caller really knows more about the inner workings of the method than it should.

Even worse, when you have an instance of User, you have to know where it came from so that you know which properties are populated. You don't want to have to know that.

Over time, when developers see User used as a container for method arguments, they might start adding properties to it for single-use scenarios. Now it's getting ugly, because the class is getting cluttered with properties that will almost always be null or default.

Such corruption isn't inevitable, but it happens over and over when we pass an object around just because we need access to a few of its properties. The danger zone is the first time you see someone creating an instance of User and just populating a few properties so they can pass it to a method. Put your foot down on it because it's a dark path.

Where possible, set the right example for the next developer by only passing what you need to pass.

score 1 · Answer 8 · answered Jun 27 '18 at 11:25

This is a really interesting question. It does depend.

If you think your method may change in the future internally to require different parameters of the User object, you should certainly pass in the whole thing. The advantage is that the code external to the method is then protected from changes within the method in terms of what parameters it is using, which as you say would cause a cascade of changes externally. So passing in the whole User increases encapsulation.

If you are pretty sure you will never need to use anything other than say the User's email, you should pass that in. The advantage of this is that you can then use the method in a wider range of contexts: you could use it for instance with a Company's email or with an email someone just typed in. This increases flexibility.

This is part of a wider range of questions about building classes to have a wide or narrow scope, including whether or not to inject dependencies and whether to have globally available objects. There is an unfortunate tendency at the moment to think narrower scope is always good. However, there is always a tradeoff between encapsulation and flexibility as in this case.

score 1 · Answer 9 · answered Jun 29 '18 at 08:02

I find it's best to pass as few parameters as possible and as many as necessary. This makes testing easier and does not require crating entire objects.

In your example, if you are going to use only the user-id or user-name then this is all you should be passing. If this pattern repeats several times and the actual user-object is much bigger then my advice is to create a smaller interface(s) for that. It could be

interface IIdentifieable
{
    Guid ID { get; }
}

or

interface INameable
{
    string Name { get; }
}

This makes testing with mocking a lot easier and you instantly know which values are really used. Otherwise you often need to initizialize complex objects with many other dependencies although at the end you just need one or two properties.

Designing a Class to take whole classes as parameters rather than individual properties

9 Answers9

Linked