Complexity

Gilbert Consellado

Ive create a method that will handle keydown event it has a lot of "if" statment and it say i has a high complexity and its scores 33 is there way to decrease its complexity actually the method is not finish here is the code

	private static void FlyouCommandEvent(object sender, KeyEventArgs e)
	{
		var fd = (FlyoutDialog)sender;
		if ((e.KeyCode == Keys.A || (e.Alt && e.KeyCode == Keys.A)) && fd.CanExecuteCommand(Helpers.FlyoutCommandAbort))
		{
			fd.ExecuteCommand(Helpers.FlyoutCommandAbort);
		}
		else if (e.KeyCode == Keys.Escape && fd.CanExecuteCommand(Helpers.FlyoutCommandCancel))
		{
			fd.ExecuteCommand(Helpers.FlyoutCommandCancel);
		}
		else if ((e.KeyCode == Keys.I || (e.Alt && e.KeyCode == Keys.I)) && fd.CanExecuteCommand(Helpers.FlyoutCommandIgnore))
		{
			fd.ExecuteCommand(Helpers.FlyoutCommandIgnore);
		}
		else if ((e.KeyCode == Keys.N || (e.Alt && e.KeyCode == Keys.N)) && fd.CanExecuteCommand(Helpers.FlyoutCommandNo))
		{
			fd.ExecuteCommand(Helpers.FlyoutCommandNo);
		}
		else if ((e.KeyCode == Keys.N || (e.Alt && e.KeyCode == Keys.N)) && fd.CanExecuteCommand(Helpers.FlyoutCommandNone))
		{
			fd.ExecuteCommand(Helpers.FlyoutCommandNone);
		}
		else if (e.KeyCode == Keys.Enter && fd.CanExecuteCommand(Helpers.FlyoutCommandOK))
		{
			fd.ExecuteCommand(Helpers.FlyoutCommandOK);
		}
		else if ((e.KeyCode == Keys.R || (e.Alt && e.KeyCode == Keys.R)) && fd.CanExecuteCommand(Helpers.FlyoutCommandRetry))
		{
			fd.ExecuteCommand(Helpers.FlyoutCommandRetry);
		}
		else if ((e.KeyCode == Keys.Y || (e.Alt && e.KeyCode == Keys.Y)) && fd.CanExecuteCommand(Helpers.FlyoutCommandYes))
		{
			fd.ExecuteCommand(Helpers.FlyoutCommandYes);
		} 
	}

any suggestion is highly appreciated thank you

Godhaniketan

there is switch....catch option is availabel as alternate option of if condition

Pete OHanlon

There's a heck of a lot of code going on here, and it certainly appears as though you're violating SRP here. Rather than doing things like this, I would look to implement separate classes that do what you need to do - each class would then decide whether it was the one that handled the case. There's even a name for this, as a pattern; it's called the Chain Of Responsibility. So, you would end up with something like this:

public abstract class HandlerBase
{
private IHandler successor;
public void SetSuccessor(IHandler successor)
{
this.successor = handler;
}
public virtual void HandleRequest(KeyEventArgs e)
{
if (CanHandleRequest(e))
{
return;
}
if (successor != null)
{
successor.HandleRequest(e);
}
}

public abstract bool CanHandleRequest(KeyEventArgs e);
public abstract void HandleRequest(FlyoutDialog fd);
}

public class FlyoutAbort : HandlerBase
{
public virtual bool CanHandleRequest(KeyEventArgs e)
{
return (e.KeyCode == Keys.A || (e.Alt && e.KeyCode == Keys.A)) && fd.CanExecuteCommand(Helpers.FlyoutCommandAbort);
}

public abstract void HandleRequest(FlyoutDialog fd)
{
fd.ExecuteCommand(Helpers.FlyoutCommandAbort); // In reality, I would look to move the command from Helpers, into this class.
}
}

public class FlyoutCancel : HandlerBase
{
public virtual bool CanHandleRequest(KeyEventArgs e)
{
return e.KeyCode == Keys.Escape && fd.CanExecuteCommand(Helpers.FlyoutCommandCancel;
}

public abstract void HandleRequest(FlyoutDialog fd)
{
fd.ExecuteCommand(Helpers.FlyoutCommandCancel);
}
}

Then, in your main class, you simply do this:

private FlyoutAbort flyoutAbort;
private FlyoutCancel flyoutCancel;

private void InitializeChain()
{
flyoutAbort = new FlyoutAbort();
flyoutCancel = new FlyoutCancel();
flyoutAbort.SetSuccessor(flyoutCancel);
}

Finally, all you need to do to handle the request (if it can be handled that is), is call:

flyoutAbort.HandleRequest(e);

Gilbert Consellado

Thank you so much, but i guess switch cant handle expressions. Correct if I am wrong.

Gilbert Consellado

Nice one, let see what I can do. Thank you so much.

Alan N

Off topic with regards to your question but is your key testing code correct? I assume the intention of the following code is to allow A or Alt-A

if ((e.KeyCode == Keys.A || (e.Alt && e.KeyCode == Keys.A))

The first condition does not test for modifier keys so would be true for A with any combination of modifiers keys (including none). Due to shortcut evaluation the second condition will be evaluated only if the first is false, that is if the Key is NOT A and I'm sure you can see the problem with that. Even if it was evaluated the condition does not exclude additional modifiers so Ctrl-Alt-A would be allowed. The solution is to test the key and the modifiers separately. Specifically the modifiers test should be for none or just Alt.

if ((e.KeyCode == Keys.A) && (e.Modifiers == Keys.None || e.Modifiers == Keys.Alt))

Hope that helps, Alan.

Gilbert Consellado

Its all works now. Thank you so much.

Gilbert Consellado

Oh I didn't realize that. Thank you so much