Should you always pass the bare minimum data needed into a function in cases like this

Question

Let's say I have a function IsAdmin that checks whether a user is an admin. Let's also say that the admin checking is done by matching user id, name and password against some sort of rule (not important).

In my head there are then two possible function signatures for this:

public bool IsAdmin(User user);
public bool IsAdmin(int id, string name, string password);

I most often go for the second type of signature, thinking that:

• The function signature gives the reader a lot more info
• The logic contained inside the function doesn't have to know about the User class
• It usually results in slightly less code inside the function

However I sometimes question this approach, and also realize that at some point it would become unwieldy. If for example a function would map between ten different object fields into a resulting bool I would obviously send in the entire object. But apart from a stark example like that I can't see a reason to pass in the actual object.

I would appreciate any arguments for either style, as well as any general observations you might offer.

I program in both object oriented and functional styles, so the question should be seen as regarding any and all idioms.

Answer 1

I personally prefer the first method of just IsAdmin(User user)

It's much easier to use, and if your criteria for IsAdmin changes at a later date (perhaps based on roles, or isActive), you don't need to rewrite your method signature everywhere.

It's also probably more secure as you aren't advertising what properties determine if a user is an Admin or not, or passing around the password property everywhere. And syrion makes a good point that what happens when your id doesn't match the name/password?

The length of code inside a function shouldn't really matter providing the method does its job, and I'd much rather have shorter and simpler application code than helper method code.

Answer 2

The first signature is superior because it allows the encapsulation of user-related logic in the User object itself. It is not beneficial to have logic for the construction of the "id, name, password" tuple strewn about your code base. Furthermore, it complicates the isAdmin function: what happens if someone passes in an id that does not match the name, for example? What if you want to check if a given User is an administrator from a context where you should not know their password?

Furthermore, as a note on your third point in favor of the style with extra arguments; it may result in "less code inside the function," but where does that logic go? It can't just vanish! Instead, it's spread around every single place where the function is called. To save five lines of code in a single place, you've paid five lines per use of the function!

Answer 3

The first, but not (only) for the reasons others have given.

public bool IsAdmin(User user);

This is type safe. Especially since User is a type you defined yourself, there is little or no opportunity to transpose or switch up arguments. It clearly operates on Users and is not some generic function accepting any int and two strings. This is a big part of the point of using a type-safe language like Java or C# which your code looks like. If you are asking about a specific language, you might want to add a tag for that language to your question.

public bool IsAdmin(int id, string name, string password);

Here, any int and two strings will do. What prevents you from transposing the name and password? The second is more work to use and allows more opportunity for error.

Presumably, both functions require that user.getId(), user.getName(), and user.getPassword() be called, either inside the function (in the first case) or before calling the function (in the second), so the amount of coupling is the same either way. Actually, since this function is only valid on Users, in Java I would eliminate all arguments and make this an instance method on User objects:

user.isAdmin();

Since this function is already tightly coupled to Users, it makes sense to make it a part of what a user is.

P.S. I'm sure this is just an example, but it looks like you are storing a password. You should instead only store a cryptographically secure password hash. During login, the supplied password should be hashed the same way and compared against the stored hash. Sending passwords around in plain-text should be avoided. If you violate this rule and isAdmin(int Str Str) were to log the user name, then if you transposed the name and password in your code, the password could be logged instead which creates a security issue (don't write passwords to logs) and is another argument in favor of passing an object instead of its component parts (or using a class method).

Answer 4

If your language of choice doesn't pass structures by value, then (User user) variant seems better. What if someday you decide to scrap the username and just use the ID/password to identify the user?

Also, this approach inevitably leads to long and overblown method calls, or inconsistencies. Is passing 3 arguments okay? How about 5? Or 6? Why think of that, if passing an object is cheap in terms of resources (even cheaper than passing 3 arguments, probably)?

And I (sort of) disagree that the second approach gives the reader more info - intuitively, the method call is asking "whether the user has admin rights", not "whether the given id/name/password combination has admin rights".

So, unless passing the User object would result in copying a large structure, the first approach is cleaner and more logical to me.

Answer 5

I would probably have both. The first one as an external API, with some hope of stability in time. The second one as a private implementation called by the external API.

If at some later time I have to change the checking rule, I would just write a new private function with a new signature called by the external one.

This has the advantage of easyness to change inner implementation. Also it's really usual that at some time you may need both functions available at the same time and call one or the other depending of some external context change (for instance you may have coexisting old Users and new Users with different fields).

Regarding the title of the question, in a way in both cases you are providing the minimum necessary information. A User seems to be the smallest Application Related Data Structure containing the information. The three fields id, password and name seems to be the smallest implementation datas actually needed, but these are not really Application level object.

In other words If you were dealing with a database a User would be a record, while id, password and login would be fields in that record.

Answer 6

There is one negative point to send classes (reference types) to methods, and that is, Mass-Assignment Vulnerability. Imagine that instead of IsAdmin(User user) method, I have UpdateUser(User user) method.

If User class has a Boolean property called IsAdmin, and if I don't check it during the execution of my method, then I'm vulnerable to mass-assignment. GitHub was attacked by this very signature in 2012.

Answer 7

I'd go with this approach:

public bool IsAdmin(this IUser user) { /* ... */ }

public class User: IUser { /* ... */ }

public interface IUser
{
    string Username {get;}
    string Password {get;}
    IID Id {get;}
}

• Using the interface type as a parameter for this function allows you to pass in User objects, to define mock User objects for testing, and gives you more flexibility around both the use and maintenance of this function.

• I also added the keyword this to make the function an extension method of all IUser classes; this way you can write code in a more OO friendly way and use this function in Linq queries. Simpler still would be to define this function in IUser and User, but I assume there's some reason why you decided to put this method outside of that class?

• I use the custom interface IID instead of int as this allows you to redefine properties of your ID; e.g. should you ever need to change from int to long, Guid, or something else. That's probably overkill, but always good to try to make things flexible such that you're not locked into early decisions.

• NB: Your IUser object can be in a different namespace and/or assembly to the User class, so you have the option to keep your User code completely separate from your IsAdmin code, with them only sharing one referenced library. Exactly what you do there is a call for how you suspect these items to be used.

Answer 8

Disclaimers:

For my reply, I'm going to focus on the style issue and forget about wether an ID, login and plain password is a good set of parameters to use, from the security point of view. You should be able to extrapolate my answer to whatever basic set of data you're using to figure out about the user's privileges...

I also consider user.isAdmin() to be equivalent to IsAdmin(User user). That's a choice for you to make.

Reply:

My recommendation is to either user only:

public bool IsAdmin(User user);

Or use both, along the lines of:

public bool IsAdmin(User user); // calls the private method below
private bool IsAdmin(int id, string name, string password);

Reasons for the public method:

When writing public methods it's usually a good idea to think about how they will be used.

For this particular case, the reason why you'd usually call this method is to figure out if a certain user is an administrator. That's a method you need. You really don't want each caller to have to pick the right parameters to send over (and risk mistakes due to code duplication).

Reasons for the private method:

The private method receiving only the minimum set of required parameters can be a good thing sometimes. Sometimes not so much.

One of the good things about splitting these methods is that you can call the private version from several public methods in the same class. For example if you wanted (for whatever reason) to have slightly different logic for Localuser and RemoteUser (maybe there's a setting to turn on/off remote admins?):

public bool IsAdmin(Localuser user); // Just call private method
public bool IsAdmin(RemoteUser user); // Check if setting is on, then call private method
private bool IsAdmin(int id, string name, string password);

Additionally, if for any reason you need to make the private method public... you can. It's as easy as just changing private to public. It may not seem like that big of an advantage for this case, but sometimes it really is.

Also, when unit-testing you'll be able to make much more atomic tests. It's usually very nice not to have to create a full user object to run tests on this method. And if you want full coverage, you can test both calls.

Answer 9

public bool IsAdmin(int id, string name, string password);

is bad for the reasons listed. That does not mean

public bool IsAdmin(User user);

is automatically good. In particular if User is an object having methods like: getOrders(), getFriends(), getAdresses(), ...

You might consider refactoring the domain with an UserCredentials type containing only id, name, password, and passing that to IsAdmin instead of all the unneeded User data.