3

I have a method which takes a single id right now. I need to add a method which takes a list of ids. My options are to have one of the methods call the other like so:

void NotifyUsers(List<int> userIds) {//do things}

void NotifyUser(int userId)
{
  NotifyUsers(new List<int>{userId})
}

Or I can change the calls to NotifyUsers(new List<int>{9999}.

Or I can use generics, etc.

What's "best practice" here?

I understand that this is somewhat of an opinion-based questions, but it seems like there should be a standard around this and I can't find it.

5
  • 3
    Maybe you are looking for the params keyword?
    – janw
    Mar 16, 2020 at 18:31
  • 1
    Why does it take a list in particular? This means that you cannot pass to your method an array, a query, or any other sequence. What exactly are you doing to the list that requires it to be specifically a list? Mar 16, 2020 at 18:32
  • Are you trying to do something with each item in userIds or something with the collection as a whole? Mar 16, 2020 at 18:32
  • @EricLippert It definitely does not need to take a list - any IEnumerable would work.
    – VSO
    Mar 16, 2020 at 18:34
  • @TylerHundley In this particular case, I am passing an Email object and sending emails async via Mailkit's SMTP. Just made an easy example because I thought it wasn't relevant.
    – VSO
    Mar 16, 2020 at 18:35

2 Answers 2

20

What's "best practice" here?

The best practice is to take a holistic approach with particular emphasis upon the needs of the caller. That does not necessarily mean providing maximum flexibility for the caller! It means understanding the use cases for the caller.

The way I would personally architect this solution is:

// Notice: User, not Users. This notifies a single user.
void NotifyUser(int id) 
{
  // notify the user
}

void NotifyUsers(IEnumerable<int> ids)
{
  foreach(var id in ids) 
    NotifyUser(id);
}

This emphasizes to the caller that if you want to do it once, you use NotifyUser, and if you have any sequence of users -- not just a list -- then you call NotifyUsers with that sequence.

Now the next question I would ask is: do the callers ever want to do this:

NotifyUsers(10, 20, 30);

In that case, I would add a third function:

void NotifyUsers(params int[] ids)
{
  NotifyUsers((IEnumerable<int>) ids);
}

This technique is flexible for the caller while ensuring that the majority of your methods are trivial one-liners. If there is a bug in NotifyUser you want to fix that in only one place.

A downside of that approach is that NotifyUsers() becomes legal and is a no-op. Someone could call it accidentally and think it was doing something. In that case you might force there to be at least one:

void NotifyUsers(int id, params int[] ids)
{
  NotifyUser(id);
  NotifyUsers((IEnumerable<int>) ids);
}

This illustrates an important point: thinking about the needs of the caller also involves psychoanalyzing people you don't know to figure out what they are going to do wrong, and then preventing it before it happens. Designing APIs that lead people naturally to only success is hard!

(Also, note that these sketches omit error handling; you probably want to check that the sequences and arrays are not null, and so on.)

The key here is: start by understanding the caller's needs; design the API that meets their needs. Then implement it. That said, what should the implementation choice be?

It depends on how notifying a sequence of users works. The solution I sketched above makes some assumptions. They are:

  • It is not more efficient to notify a hundred users "at once" than it is to notify a hundred users one at a time.
  • If notification of one user in a sequence fails, the right thing to do is to stop notifying any more.

What if those assumptions are wrong? Consider an API which updates a database, and the expensive part is not the update but making the connection to the database. In that case you do NOT want to write:

void NotifyUser(int id) 
{
  Connect(); // Expensive
  Update(id);
  Disconnect();
}
void NotifyUsers(IEnumerable<int> ids)
{
  foreach(var id in ids)
    NotifyUser(id);
}

Because now you are doing 100 connections for 100 users. Instead you want to flip the script:

void NotifyUser(int id) 
{
  NotifyUsers(Enumerable.Repeat(id, 1));
}
void NotifyUsers(IEnumerable<int> ids)
{
  Connect();
  foreach(var id in ids)
    Update(id);
  Disconnect();
}

Notice that in rewriting the implementation, I did not rewrite the API; remember, we've already designed the API to be good for callers, so if this API meets their needs, don't change it! Methods are abstractions; we can change the details to meet performance requirements. (And if we cannot meet perf goals because of an API design issue, then we didn't design an API that met caller needs in the first place.)

And now what about error handling? There are numerous possibilities:

  • Notifications are required to be best-effort. If one notification in a sequence fails, catch the exception, eat it, and keep going with the other users. Never inform the caller of failure
  • Notifications are required to be best-effort, but callers are required to know about all failures. Log the exceptions and either re-throw them or return a failure report instead of void.
  • Notifications are required to be all-or-nothing. That is, if the tenth notification fails, then undo the previous notifications to keep the world consistent. This is hard. You can't un-ring a bell.

And so on. Again think very carefully about the needs of the caller. What do they expect will happen on failure? Do you know how to write the logic to do what the caller expects? Can you clearly document it so that the caller knows whether their expectations are met?

3
  • 2
    Accepting this for completeness. Appreciate the help everyone.
    – VSO
    Mar 16, 2020 at 18:48
  • In method using the params keyword, is this casting really necessary: NotifyUsers((IEnumerable<int>) ids)? Isn't ids already an IEnumerable<int>? Jun 3, 2020 at 21:04
  • 2
    @LucaCremonesi: In the version of the method void NotifyUsers(params int[] ids) { NotifyUsers((IEnumerable<int>) ids); } the cast is necessary because without it, you end up in an unbounded recursion! A params method may legally be called in its "unexpanded" form as well as its "expanded" form. Nov 20, 2020 at 20:50
5

If an array is acceptable in lieu of specifically a List<> then you can use the params keyword. For example:

void NotifyUsers(params int[] userIds)
{
    //...
}

Which you could call with zero or more arguments:

NotifyUsers(123);
NotifyUsers(5, 67, 890);
// etc.
6
  • The OP explicitely asked for an IEnumerable<T>, so it would rather be NotifyUsers(list.ToArray()).
    – janw
    Mar 16, 2020 at 18:34
  • @JanWichelmann: The original poster also said in a comment that actually they want to accept any sequence, not just lists. Mar 16, 2020 at 18:35
  • This seems to be what I am looking for - I had no idea about the params keyword...shame on me.
    – VSO
    Mar 16, 2020 at 18:36
  • @EricLippert Yeah, that was a race condition when posting my comment ;) Still, ToArray should do the trick. I have edited the comment, but an edit of the answer might be desirable.
    – janw
    Mar 16, 2020 at 18:37
  • 3
    It sounds like a secondary lesson here is also to use the simplest types to support only the truly necessary functionality in method signatures. (e.g. Using an IEnumerable<> instead of a List<> when all that's needed is the ability to enumerate the collection)
    – David
    Mar 16, 2020 at 18:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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