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.

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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".

Answer 5

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).

Answer 6

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.

Answer 7

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.

Answer 8

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.

Answer 9

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.