Skip to main content
Improved the answer format.
Source Link

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

NOTE: I consider user.isAdmin() to be equivalent to IsAdmin(User user). That's a choice for you to make.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 places if neededpublic methods in the same class. This is not a particularly good For example for that, as you'll probably notif you wanted (for whatever reason) to have other methods that share thisslightly different logic - but for more generic code snippets it can beLocaluser and RemoteUser (maybe there's a very good thing.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 beseem like that big of an advantage for this case, but for otherssometimes it can bereally 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.

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

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

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

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

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 places if needed. This is not a particularly good example for that, as you'll probably not have other methods that share this logic - but for more generic code snippets it can be a very good thing.

Additionally, if for any reason you need to make the private method public... you can. It may not be that big of an advantage for this case, but for others it can be.

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.

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.

Source Link

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

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

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

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

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 places if needed. This is not a particularly good example for that, as you'll probably not have other methods that share this logic - but for more generic code snippets it can be a very good thing.

Additionally, if for any reason you need to make the private method public... you can. It may not be that big of an advantage for this case, but for others it can be.

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.