Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
33  
Off-topic: Out of curiousity, why does the "is admin" status of a user depend on their password? –  stakx Nov 3 '13 at 15:45
40  
@syb0rg Wrong. In C# that's the naming convention. –  atticae Nov 3 '13 at 17:14
64  
@syb0rg Right, he did not specify the language so I think its rather odd to tell him he is violating the convention of a certain language. –  atticae Nov 3 '13 at 17:22
31  
@syb0rg The general statement that "function names shouldn't start with an uppercase letter" is wrong, because there are languages where they should. Anyway, I did not mean to offend you, I was just trying to clear this up. This is getting pretty much off-topic to the question itself. I dont see the need, but if you want to continue lets move the debate to the chat. –  atticae Nov 3 '13 at 17:41
6  
Just as a general point, rolling your own security is usually a bad thing(tm). Most languages and frameworks have a security / permission systems available and there are good reasons for using them even when you want something simpler. –  James Snell Nov 3 '13 at 22:44
show 5 more comments

8 Answers 8

up vote 116 down vote accepted

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.

share|improve this answer
3  
I think the refactoring issue is a very good point - thank you. –  Anders Holmström Nov 3 '13 at 14:41
1  
I was going to make the method signature argument if you didn't. This is especially helpful when there's a lot of dependencies on your method that are maintained by other programmers. This helps keep people out of each other's way. +1 –  jmort253 Nov 3 '13 at 20:35
1  
I agree with this answer, but let me flag two situations where the other approach ("minimum data") might be preferable: (1) You want to cache method results. Better not to use a whole object as the cache key, or have to check an object's properties on every invocation. (2) You want the method/function to execute asynchronously, which might involve serializing the arguments and, say, storing them in a table. Easier and simpler to look up an integer than an object blob when managing pending jobs. –  GladstoneKeep Nov 4 '13 at 15:50
    
The primitive obsession anti-pattern could be horrible for unit testing. –  Samuel Nov 5 '13 at 23:03
add comment

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!

share|improve this answer
40  
Following your logic, wouldn't user.IsAdmin() be a lot better? –  Anders Holmström Nov 3 '13 at 14:37
11  
Yes, absolutely. –  syrion Nov 3 '13 at 14:38
5  
@AndersHolmström It depends, are you testing if the user is an admin in general, or just an admin for the current page/form you're on. If in general, then yes that would be better, but if you're only testing IsAdmin for a specific form or function, that should be unrelated to the user –  Rachel Nov 3 '13 at 14:41
34  
@Anders: no it shouldn't. A user shouldn't decide whether it is an Admin or not. The Privileges or Security system should. Something being tightly coupled to a class doesn't mean it is a good idea to make it part of that class –  Marjan Venema Nov 3 '13 at 19:44
7  
@MarjanVenema - the idea that the User class must not "decide" whether an instance is an admin strikes me as being a bit cargo-cultish. You really can't make that generalization so strongly. If your needs are complex, then perhaps it's beneficial to encapsulate them in a separate "system", but citing KISS+YAGNI I'd like to make that decision actually faced with that complexity, and not as a general rule :-). And note that even if the logic is not "part" of User it can still be useful as a matter of practicality to have a passthrough on User that just delegates to a more complex system. –  Eamon Nerbonne Nov 4 '13 at 21:39
show 16 more comments

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

share|improve this answer
10  
A user shouldn't decide whether it is an Admin or not. The Privileges or Security system should. Something being tightly coupled to a class doesn't mean it is a good idea to make it part of that class. –  Marjan Venema Nov 3 '13 at 19:43
4  
@MarjanVenema The user isn't deciding anything. The User object/table stores data about each user. Actual users don't get to change everything about themselves. Even when the user changes something they are allowed to, it may trigger security alerts like an email if they change their email address or their user ID or password. Are you using a system where users are magically allowed to change everything about certain object types? I am not familiar with such a system. –  GlenPeterson Nov 3 '13 at 23:57
2  
@GlenPeterson: are you deliberately misunderstanding me? When I said user, I of course meant the user class. –  Marjan Venema Nov 4 '13 at 7:32
2  
@GlenPeterson Totally off topic, but multiple rounds of hashing and cryptographic functions will weaken the data. –  Gusdor Nov 4 '13 at 9:36
3  
@MarjanVenema: I am not deliberately being difficult. I think we just have very different perspectives and I would like to understand yours better. I have opened a new question where this can be better discussed: programmers.stackexchange.com/questions/216443/… –  GlenPeterson Nov 4 '13 at 12:17
show 8 more comments

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.

share|improve this answer
add comment

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.

share|improve this answer
    
I don't see the reason for private method. Why maintain both? If private method needs different arguments you will still have to modify public one. And you will test it through public one. And no it's not usual that you need both at some time later. You are guessing here - YAGNI. –  Peri Nov 7 '13 at 22:14
    
You are assuming for instance that the User data structure does not have any other field from the beginning. It is usually not true. The second functions makes clear which inner part of the user is checked to see if it's an admin. Typical use cases may be of the kind : if user creation time is before some date, then call old rule, if not call new rule (which may depend on other parts of User, or depends on the same but using another rule). Splitting the code this way you'll get two minimal functions: a minimal conceptual API and a minimal implementation function. –  kriss Nov 8 '13 at 17:01
    
in other words this way it will be immediately visible from the internal function signature which parts of User are used or not. In production code that is not always obvious. I'm currently working on a large codebase with several years maintenance history and this kind of splitting is very useful for long terme maintenance. If you don't intend to maintain the code, it's indeed useless (but even providing a stable conceptual API is also probably useless). –  kriss Nov 8 '13 at 17:05
    
Another word: I certainly won't unit-test the internal method through external one. That's bad testing practice. –  kriss Nov 8 '13 at 17:07
1  
This is a good practice, and there's a name for it. It's called the "single responsibility principle". It can be applied to both classes and methods. Having methods with a single responsibility is a good way to make more modular and maintainable code. In this case, one task is picking basic set of data from the user object, and another task is checking that basic set of data to see if the user is an administrator. By following this principle you get the bonus of being able to compose methods and avoid code duplication. This also means, as stated by @kriss that you get better unit tests. –  diegoreymendez May 10 at 18:06
show 7 more comments

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.

share|improve this answer
add comment

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.

share|improve this answer
2  
IUser is useless here and only adds complexity. I don't see why you couldn't use real User in tests. –  Peri Nov 7 '13 at 22:11
1  
It adds future flexibility for very little effort, so whilst not adding value currently it doesn't hurt. Personally I prefer to always do this since it avoids having to think through future possibilities for all scenarios (which is pretty much impossible given the unpredictable nature of requirements, and will take a lot more time to even get a partially good answer than it would to stick in an interface). –  JohnLBevan May 11 at 19:27
    
@Peri a real User class might be complex to create, taking an interface allows you to mock it out so you can keep your tests simple –  jk. May 12 at 14:09
    
@JohnLBevan: YAGNI!!! –  Peri May 13 at 20:57
    
@jk.: if it's hard to build User then something is wrong –  Peri May 13 at 20:59
add comment

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.

share|improve this answer
add comment

protected by gnat May 12 at 13:36

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.