How to reduce this IF-Else ladder in c#

Question

This is the IF -Else ladder which I have created to focus first visible control on my form.According to the requirement any control can be hidden on the form.So i had to find first visible control and focus it.

 if (ddlTranscriptionMethod.Visible)
    {
        ddlTranscriptionMethod.Focus();
    }
    else if (ddlSpeechRecognition.Visible)
    {
        ddlSpeechRecognition.Focus();
    }
    else if (!SliderControl1.SliderDisable)
    {
        SliderControl1.Focus();
    }
    else if (ddlESignature.Visible)
    {
        ddlESignature.Focus();
    }
    else
    {
        if (tblDistributionMethods.Visible)
        {
            if (chkViaFax.Visible)
            {
                chkViaFax.Focus();
            }
            else if (chkViaInterface.Visible)
            {
                chkViaInterface.Focus();
            }
            else if (chkViaPrint.Visible)
            {
                chkViaPrint.Focus();
            }
            else
            {
                chkViaSelfService.Focus();
            }
        }
    }

Is there any other way of doing this. I thought using LINQ will hog the performance as i have to tranverse the whole page collection. I am deep on page which has masterpages.Please suggest.

Jareb

Leave it as is. Unless the list of controls changes frequently, you will have a hard time getting this any prettier. See the solutions below: They are just variations on what has to be done. Pat your shoulder. Move on to some interesting, meaty problems to solve! Don't waste your brain on this stuff... – Numeral

It is a curiosity to write best code and learn from the experience of people using SO. – Jareb

Answer 1

I think your tree is good. This certainly looks like a logic tree that can be simplified, and you have a good sense of smell to be suspicious of it. However, it seems to be that the logic tree reflects what you need. The logic really is this convoluted, and this is the conditional framework that C# gives you to handle this situation. I don't think it can be improved.

If you had a simple list of controls that should have the focus, and you wanted to give focus to the first visible control in the list, you could do this:

(From c in ListOfControls
Where c.visible = true
Select c).First.Focus();

But, it appears you have some additional criteria, so that wouldn't work.

Wollastonite

I don't know why this is getting downvoted. I think you have a valid point. This code isn't "pretty" but I don't think it is all that terrible either. Plus, it has the benefit of maintainability in that it's completely obvious what he is trying to accomplish – Demonic

@Kevin. Yeah, the downvotes surprised me also. If his situation was more simple, then yes, we could make the code more simple. But he's asking "what can I do given these requirements.", not asking "what could I do if my situation was simpler?" I guess some feel I'm being defeatist? :) – Wollastonite

I agree. The OP code is probably as close to what is being done as possible. Sometimes we tend to go crazy trying to refactor stuff like this. Just because it isn't pretty. Well, it doesn't matter either. And you should move on to the next task. – Numeral

+1 Down voters should leave comments, in my humble opinion. I wish SO would show who up/down vote answers. – Trenttrento

+1 Agreed. It's easy to get hung-up on writing elegant code, in this case it's time that can be better spent elsewhere. – Nitrogenous

I think that this is slow and eat memory. Avoid using so much linq. (I do not have vote). – Gypsy

Answer 2

Two approaches:

1. Iterate controls and set focus if visible
2. Use TabIndex and set focus to first. Then focus should fall to first visible control

Cu

Please put some light on 2nd part. – Jareb

Could you elaborate on these, especially #2? How would it deal with .SliderDisable, and the 4 controls that should only be given the focus if tblDistributionMethods.Visible is true, but tblDistributionMethods should not be given focus? What's the code to handle his currently logic more concisely? – Wollastonite

there is property TabIndex msdn.microsoft.com/library/… for every control. it defines sequence when you press Tab key while control in focus. i just thought that it might affect setting focus too. – Cu

Answer 3

If you're just trying to focus the first visible control on the form, then I would replace the entire ladder with a single loop:

foreach (Control c in Controls)
{
    if (c.Visible)
    {
        c.Focus();
        break;
    }
}

If you need to focus an inner control, use a recursive method:

bool FocusFirst(ControlCollection controls)
{
    foreach (Control c in controls)
    {
        if (c.Visible)
        {
            c.Focus();
            FocusFirst(c.Controls);
            break;
        }
    }
}

Commines

There's a problem with that in that you can't control the ordering of the controls you'd want to check. – Feld

@CAbbott: Please reread the question. ...which I have created to focus first visible control on my form. This does exactly what he asks. – Commines

But he's checking visibility of the controls in a specific order (probably the order that they appear on the page), the controls collection would be in the order that they were added in to the collection. I'm just saying that you couldn't guarantee any specific order using the controls collection. – Feld

@CAbbott: Did you read the question or just the code? Nowhere does it say he needs to guarantee any specific order; just "focus the first control." – Commines

Both, I inferred a specific order because otherwise it wouldn't make much sense. Why would he want to set focus to the first visible control that may be in the middle of the page? Just sayin... – Feld

@Feld - if the focus needs to be applied in a particular order, then it would be trivial to create a control in which the correct order is defined and pass that to Aaronaught’s method. – Pasteurize

@CAbbott: How do you figure that the first visible control would be in the middle of the page? Some sort of absolute positioning madness? Whatever you inferred was not listed in the requirements; if a specific order is, in fact, a requirement, then the OP is free to add that as a clarification, at which point I'll put in a 1-line edit to have the method take a params Control[] controls argument. – Commines

@Jeffrey - agreed. My original comment was based on Aaronaught's initial post of a for loop going through a controls collection; my assumption (right or wrong) was that it would be iterating through probably the Page.Controls collection - in which case it would still have problems. Any ways, I never meant to turn this in to a thing. – Feld

@Commines - It's not unheard of to use a <div> tag or a PlaceHolder or a style sheet in general to control display order. At any rate, that's about all I'll say on the matter. – Feld

@Feld - I don't think we're done here yet! Oh, wait, maybe we are. Cool. – Pasteurize

Answer 4

You could return after you meet your criteria, for example:

   if (ddlTranscriptionMethod.Visible) 
    { 
        ddlTranscriptionMethod.Focus(); 
        return;
    }

    if (ddlSpeechRecognition.Visible) 
    { 
        ddlSpeechRecognition.Focus(); 
        return;
    } 

etc..

Hexachlorophene

I don't really see how this is better. The problem isn't with the else if statements, those aren't nested; this is just littering the method with a bunch of extra return statements. – Commines

+1 I personally like the 'return'. Seems a lot of people don't but I find it perfectly acceptable and very useful. – Trenttrento

@J.Hendrix: I have absolutely nothing against early returns; it's just that these early returns don't actually improve the readability, since they're not breaking any nesting. – Commines

@Aaronaught, I disagree. The return will likely improve performance and for me at least, will also improve readability. But I have to concede that the performance boost is prob near nill, so it is more or less a personal preference. – Trenttrento

I wonder how the controls are being set visible/enabled? I wonder if the focus logic would be better placed there? – Trenttrento

@J.Hendrix: Returning early won't make any difference here because the remaining else ifs never get tested anyway. – Bastardize

Answer 5

You can iterate controls and set focus if visible. But I would suggest you to use state pattern for better code readability.

Simasimah

HMMM looks like the final piece of this example is all about the MONIES – Zestful

@Laurence Burke, you don't have to buy a book to understand state pattern, but it's very good reading. – Chaing

+1 a clear case for state pattern if you want to solve this with OOD, and nothing wrong with linking a source that provides information and examples but leaves it's advanced samples/information behind a paywall. – Tourniquet

Answer 6

All your doing is setting the focus which is client-side functionality. I would personally do this in javascript (using jQuery). ASP.NET controls that aren't set to visible aren't rendered in the HTML, so you could look for the existence of those elements or there might be an easier way if you're just looking for the first visible, enabled control.

Fewness

Answer 7

What about a jumpto seems like you could use that here.

Zestful

Answer 8

When the list of items to evaluate is large (and sometimes it's very large), I try to separate the order of evaluation from the conditional logic, something like this:

List<WebControl> wcBasics = new List<WebControl>();
wcBasics.Add(ddlTranscriptionMethod);
wcBasics.Add(ddlSpeechRecognition);
wcBasics.Add(ddlESignature);

List<CheckBox> checks = new List<CheckBox>();
checks.Add(chkViaFax);
checks.Add(chkViaInterface);
checks.Add(chkViaPrint);

private void Focus()
{
    foreach (WebControl c in wcBasics)
        if (c.Visible) {
            c.Focus();
            return;
        }

    if (!tblDistributionMethods.Visible) return;

    foreach (CheckBox chk in checks)
        if (chk.Visible) {
            chk.Focus();
            return;
        }
    }

    chkViaSelfService.Focus();
}

Liatris

Answer 9

This is a classic state machine. By setting up a machine it can make the code easier to understand and maintain, though it may add to the total lines.

I won't get into the specifics of your code. By determining what state the user is operating in, you can programmatically change the differing values in an understandable specific state fashion. (Pseudo code below)

enum FormStates
{
    Initial_View
    Working_View
    Edit_View
    Shutdown_View
};

{ // Somewhere in code

switch (theCurrentState)
{

    case Initial_View :
        Control1.Enabled = true;
        Control2.Enabled = true;
        theCurrentState = Working_View;
    break;

   case Working_View
      if (string.empty(Contro1.Text) == false)
      {
          Control2.Enabled = false;
          Speachcontrol.Focus();
          theCurrentState = Edit_view;
      }
      else // Control 2 is operational
      {
         Control1.Enabled = false;
         SliderControl.Focus();
      }

    case Edit_View:
       ...
    break;  

   break;
}

By organizing the code in logical steps, it makes it easier to add more states without jeopardizing an huge if/else.

Beatnik

Answer 10

Here is a slightly different take on the problem. First, define an interface to represent controls which may or may not be focused:

public interface IFormControl
{
    bool Focus();
}

Then create an implementation which handles the easy cases:

public class FormControl : IFormControl
{
    private readonly Control _control;

    public FormControl(Control control)
    {
        _control = control;
    }

    public bool Focus()
    {
        if(_control.Visible)
        {
            _control.Focus();
        }

        return _control.Visible;
    }
}

And create another which handles the more difficult cases:

public class DependentFormControl : IFormControl
{
    private readonly Control _control;
    private readonly Func<bool> _prerequisite;

    public DependentFormControl(Control control, Func<bool> prerequisite)
    {
        _control = control;
        _prerequisite = prerequisite;
    }

    public bool Focus()
    {
        var focused = _prerequisite() && _control.Visible;

        if(focused)
        {
            _control.Focus();
        }

        return focused;
    }
}

Then, create an extension method which sets the focus on the first control in a sequence:

public static void FocusFirst(this IEnumerable<IFormControl> formControls)
{
    var focused = false;

    foreach(var formControl in formControls)
    {
        if(formControl.Focus())
        {
            break;
        }
    }
}

And finally, create the set of controls to be focused:

var controls = new FormControl[]
{
    new FormControl(ddlTranscriptionMethod),
    new FormControl(ddlSpeechRecognition),
    new DependentFormControl(SliderControl1, () => !SliderControl1.SliderDisable),
    new FormControl(ddlESignature),
    new DependentFormControl(chkViaFax, () => tblDistributionMethods.Visible),
    new DependentFormControl(chkViaInterface, () => tblDistributionMethods.Visible),
    new DependentFormControl(chkViaPrint, () => tblDistributionMethods.Visible),
    new DependentFormControl(chkViaSelfService, () => tblDistributionMethods.Visible)
};

controls.FocusFirst();

You can create the set of controls when your page loads and just call .FocusFirst() whenever necessary.

Weakfish