0

How should I fix SonarLint Rule S1172 "Unused method parameters should be removed" when I create EventHandler methods.

public void Subscribe()
{
    MyEvent += OnMyEvent;
}

public void UnSubscribe()
{
    MyEvent -= OnMyEvent;
}

private void OnMyEvent(object sender, EventArgs e)
{
   DoSomething();
}

You could rewrite the code with Reactive Extensions and making 'Observables' but that is quite complex solution for simple event handlers. Another option could be to rewrite the code like:

public void Subscribe()
{
   MyEvent += (s,e) => DoSomething();
}

But the question then is how do you do the UnSubscribe()? By my opinion the unused parameters is not applicable to event handler methods. But it might be difficult to make detection for that in SonarLint.

Niek Jannink
  • 1,056
  • 2
  • 12
  • 24
  • As mentioned below, storing the delegate in a field is an option. But I think we should handle this issue properly in SonarLint if this is a common scenario. My feeling is that if you don't need the `sender` and the `EventArgs` at all, then you could use a custom delegate which doesn't have those. But this only works if you control the type of the event. Is this the case? Do you have subscribers that need those two parameters? Do you think this goes against event handling best practices? – Tamas Aug 19 '15 at 06:32
  • I think its a design guideline that events always should be (derived) of the type `EventHandler`. So that means you will always get the sender and `EventArgs`. So even tho the subscribers don't use those parameters all events in the .Net framework are build using this paradigm and so SonarLint should be able to handle this. I think resharper correctly recognises event handling methods and ignores the unused parameters of these. – Niek Jannink Aug 19 '15 at 09:23
  • Thanks, let's continue the discussion on http://github.com/SonarSource/sonarlint-vs/issues/211 – Tamas Aug 19 '15 at 15:14
  • This seems a C# or VisualStudio `feature`, but indeed raises other questions, see my question: http://stackoverflow.com/q/41162335/1845672 . btw the discussion link at github is dead – Roland Dec 15 '16 at 11:22

1 Answers1

1

If you need to unsubscribe, you'll need to store the delegate (remove static for proper code, this is pasted from a hacked console app project):

public static event EventHandler TestEvent;

private static EventHandler saved = (s, e) => DoSomething();

static void Main(string[] args)
{
    TestEvent += saved;
    TestEvent -= saved;
}

internal static void DoSomething()
{
}

Or use a mass-unsubscribe:

foreach (Delegate d in TestEvent.GetInvocationList())
{
    TestEvent -= (EventHandler)d;
}

Or if you own the event, you could also use this to unsubscribe all:

TestEvent = null;

Or just use the syntax you've always used and create a non-anonymous method, like you show above. There's nothing wrong with that syntax. You could do the obligatory 

if (sender == null) 
    throw ArgumentNullException(nameof(sender));

to get rid of the warning ;)

Community
  • 1
  • 1
jessehouwing
  • 106,458
  • 22
  • 256
  • 341
  • PS: I think this should be treated as a bug in S1172. – jessehouwing Aug 18 '15 at 17:47
  • This doesn't solve my issue. Having to store the EventHandler in a field to resolve the Rule warning S1172 doesn't seem the correct way to resolve the warning. So yea I would also say its a bug in S1172 – Niek Jannink Aug 18 '15 at 18:52
  • Just add the ArgumentNullChecks ;). If you do start to use the parameters, it's the next SonarLint error you'll run into ;). – jessehouwing Aug 18 '15 at 18:54
  • Raised over at the SonarLint issues on GitHub: https://github.com/SonarSource/sonarlint-vs/issues/211 – jessehouwing Aug 19 '15 at 08:02