Correct Usage of ArgumentException?

Question

From what I've seen, ArgumentExceptions are usually used like such:

public void UpdateUser(User user)
{
    if (user == null) throw new ArgumentException("user");
    // etc...
}

but what if I have something like this:

public void UpdateUser(int idOfUser)
{
    var user = GetUserById(idOfUser);
    if (user == null) throw new ArgumentException("idOfUser");
    // etc...
}

Is that still an ArgumentException?

Answer 1

As the name suggests, an ArgumentException is an exception about an argument. It means the argument was somehow inherently wrong.

The general form is:

public void SomeMethod(SomeType arg)
{
  if(!TestArgValid(arg))
    throw new ArgumentException("arg"); //Or more specific is possible
                                        //e.g. ArgumentNullException
    /* Actually do stuff */
}

If the only possible way that GetUserById could fail was that there was something inherently incorrect with the value of idOfUser then the following would both be the same in practice:

public void UpdateUser(int idOfUser)
{
  if(!TestValid(idOfUser))
    throw new ArgumentException("idOfUser");
  var user = GetUserById(idOfUser);
  // Do stuff with user
}

public void UpdateUser(int idOfUser)
{
  var user = GetUserById(idOfUser);
  if(user == null)
    throw new ArgumentException("idOfUser");
  // Do stuff with user
}

And if it turned out to be for some reason faster or less wasteful of some resource to test user after the fact than idOfUser before the fact and if there were no side-effects of calling GetUserById, and if the difference actually mattered then maybe the second version would be a reasonable optimisation of the first.

But that only holds if all of the ifs above hold, and it's then a weird way of detecting an invalid argument that has some specific advantage where we benefit from the encapsulation of methods by hiding that weirdness from everything else.

Chances are there could be a valid idOfUser for which there was no corresponding user, in which case it certainly wasn't an argument exception.

Answer 2

The first

if (user == null) throw new ArgumentException("user");

should be

if (user == null) throw new ArgumentNullException("user");

If possible you shouldn't throw ArgumentException directly

The primary derived classes of ArgumentException are ArgumentNullException and ArgumentOutOfRangeException. These derived classes should be used instead of ArgumentException, except in situations where neither of the derived classes is acceptable.

For the second example, here Should I throw a KeyNotFoundException for a database lookup? they suggest (in comments)

if (user == null) throw new ObjectNotFoundException();

It is defined in System.Data: System.Data.ObjectNotFoundException.