CODEWITHSUNDEEP
c#asp.net
James Wilson
James Wilson

Reputation: 5150

Is there a better method to structure this if statement

It just seems a mess to me, my mind tells me there has to be a better way.

I have 6 controls on a web page.

if (printer_make_1.Text != "" && printer_model_1.Text != "" && printer_make_2.Text != "" && printer_model_2.Text != "" && printer_make_3.Text != "" && printer_model_3.Text != "")
{
  // Do something
}

What is the best/most efficient way to do this?

Upvotes: 4

Views: 190

Answers (7)

Henk Holterman
Henk Holterman

Reputation: 273274

Restructuring starts with your data: avoid printer_make_1, printer_make_2, ...

class PrinterData
{
   public string Make { get; set; }
   public string Model { get; set; }
}

PrinterData[] printers = new PrinterData[3];  //or use a List<>

printers[0] = new PrinterData { Make = "PH", Model = "1A" };
...

if (printers.All(p => ! (p.Make == "" || p.Model == "")) )
  ...

Upvotes: 5

YoryeNathan
YoryeNathan

Reputation: 14522

private bool CheckAllEmpty(params TextBox[] textBoxes)
{
    return textBoxes.All(tb => tb.Text != null);
}

private void Foo()
{
    ...

    if (CheckAllEmpty(tb1, tb2, tb3))
    {
        mbox("tb1, tb2 and tb3 are all empty");
    }
    else
    {
        mbox("tb1, tb2 or tb3 isn't empty");
    }

    ...
}

Upvotes: 0

Yuriy Faktorovich
Yuriy Faktorovich

Reputation: 68687

if (!Enumerable.Range(1, 3)
    .Any(i => ((TextBox)FindControl("printer_make_" + i)).Text == "" || 
        ((TextBox)FindControl("printer_model_" + i)).Text == "") {...}

It allows you to later expand the number of printer makes and models, but isn't as strong typed.

Upvotes: 0

James Hill
James Hill

Reputation: 61812

There are lots of ways to accomplish this - none of them are elegant. Do what is most readable to you (and those who may come behind you).

I would probably take this approach:

string makeText = String.Concat(printer_make_1.Text, printer_make_2.Text, printer_make_3.Text);
string modelText = String.Concat(printer_model_1.Text, printer_model_2.Text, printer_model_3.Text);

if (makeText.Length != 0 && modelText.Length != 0)
{
    // Do something
}

Upvotes: 1

Jeff Cuscutis
Jeff Cuscutis

Reputation: 11637

I normally put that test into a method and call that to make the if easier to read

private boolean AreAllPrinterFieldsFilled()
{
    return (printer_make_1.Text != "" 
        && printer_model_1.Text != "" 
        && printer_make_2.Text != "" 
        && printer_model_2.Text != "" 
        && printer_make_3.Text != "" 
        && printer_model_3.Text != "");
}

Then in the if:

if (AreAllPrinterFieldsFilled)
{
    // Do something
}

Upvotes: 2

Yuck
Yuck

Reputation: 50855

You can refactor into a method, if you want to improve the readability or use the same logic elsewhere:

public Boolean AllControlsHaveAValue() {
    return (printer_make_1.Text != ""
        && printer_model_1.Text != ""
        && printer_make_2.Text != ""
        && printer_model_2.Text != ""
        && printer_make_3.Text != ""
        && printer_model_3.Text != "");
}

Then just ask:

if (AllControlsHaveAValue()) {
    // do something
}

Upvotes: 6

Lee
Lee

Reputation: 144136

if(new[] { printer_make_1, printer_model_1 ...}.All(l => l.Text != string.Empty)
{
    //do something
}

You might want to split it up to be more readable:

var labels = new[] { printer_make_1, printer_model_1 ... };
if(labels.All(l => l.Text != string.Empty))
{
    //do something
}

Upvotes: 3

Related Questions