Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My code does what I need it to do, but I think I am missing the "right" way to do this with comparing the arrays. Essentially what I need this to do is compare a textbox entry against two different arrays and redirect based on which value matches.

Protected void Button1_Click(object sender, EventArgs e)
{              
      string[] str1 = new string[] {"87002", "87001", "87005"};
      string[] str2 = new string[] {"97002", "97003", "97004"};

      for (int i = 0; i < str1.Length; i++)
      {
          string comp1 = str1[i];
          if (comp1 == TextBox1.Text.ToString())
          {
              Response.Redirect("Page1.aspx");
          }
      }

      for (int i = 0; i < str2.Length; i++)
      {
          string comp2 = str2[i];
          if (comp2 == TextBox1.Text.ToString())
          {
              Response.Redirect("Page2.aspx");
          }
      }          
      Response.Redirect("Unknown.aspx");
}
share|improve this question
1  
I'd prefer List<string> to arrays, and use the Contains method instead of looping. Other than that... what exactly are you asking? Is there something wrong with your code? –  Dan J May 17 '12 at 23:41
1  
I think OP may be looking for the Array.Exists method? –  Mark M May 17 '12 at 23:42
    
basically, what is the right way to compare two strings? nothing wrong with the code, I just feel uncomfortable with it. –  mtnwk May 17 '12 at 23:43
1  
Is the question "how to determine whether a string is in an array?" or "how to compare two strings?" –  ptay89 May 17 '12 at 23:48
    
how to determine whether a string is in an array –  mtnwk May 17 '12 at 23:51
add comment

migrated from stackoverflow.com May 18 '12 at 11:51

This question came from our site for professional and enthusiast programmers.

5 Answers

up vote 13 down vote accepted

I would do this:

protected void Button1_Click(object sender, EventArgs e)
{              
    string[] str1 = new string[] {"87002", "87001", "87005"};
    string[] str2 = new string[] {"97002", "97003", "97004"};

    if (str1.Contains(TextBox1.Text))
        Response.Redirect("Page1.aspx");

    if (str2.Contains(TextBox1.Text))
        Response.Redirect("Page2.aspx");

    Response.Redirect("Unknown.aspx");
}

You don't need the ToString() calls - TextBox.Text should already be a string.

share|improve this answer
1  
I would do this, but also use arrays of int (assuming those values are not just for demonstration.) If TextBox1.Text does not parse as an int, you can go straight to Unknown.aspx without searching. –  mafue May 17 '12 at 23:56
add comment

Would you not be better using the array.Contains(..) method? For example: str1.Contains(TextBox1.Text); should suffice to check whether the string is within the array. It might be worth also storing the two string arrays as HashSet also for speed.

share|improve this answer
    
+1 for mentioning HashSet, but I don't understand "for speed if they're not very big". The benefit of using HashSet will be greater for a larger set of values; that phrase seems to imply that HashSet would only be useful for a relatively small set of values. –  phoog May 18 '12 at 15:46
    
I'm also unsure what I meant by that..removed it now. –  ptay89 May 18 '12 at 15:53
add comment

You are right that should work, however there is a cleaner way to write this using Linq. Now if you are not using 3.5+ then you will stick with your way.

Try below.

protected void btn1_Click(object sender, EventArgs e)
        {
            string[] str1 = new string[] { "87002", "87001", "87005" },
                     str2 = new string[] { "97002", "97003", "97004" };

            string txt = txtBox1.Text.Trim();

            if (str1.Any(x => x.Equals(txt)))
                Response.Redirect("Page1.aspx");
            else if (str2.Any(x => x.Equals(txt)))
                Response.Redirect("Page2.aspx");
            else
                Response.Redirect("Unknown.aspx");
        }

Unfortunatly we cant avoid a loop here but what we have done is used the "ANY" method of the string array. The "any" method returns a boolean (true\false) if any of the values in the Collection matches your lambada expression. In this instance we are simply saying does x (the item in the collection) match the txt (from txtBox1).

If it does we can redirect to the correct location.

I hope this helps

share|improve this answer
add comment

How about using Switch

 switch(input)
 {
   case foo: do something;
         break;

    case bar: do something;
         break;
 }
share|improve this answer
add comment

I would have used a Dictionary<string, string>:

private Dictionary<string, string> redirects = new Dictionary<string, string>
{
    { "87002", "Page1.aspx" },
    { "87001", "Page1.aspx" },
    { "87005", "Page1.aspx" },
    { "97002", "Page2.aspx" },
    { "97003", "Page2.aspx" },
    { "97004", "Page2.aspx" },
};

protected void Button1_Click(object sender, EventArgs e)
{              
      if (redirects.ContainsKey(TextBox1.Text))
      {
          Response.Redirect(redirects[TextBox1.Text]);
      }
      else
      {     
          Response.Redirect("Unknown.aspx");
      }
}

This solution separates the data from the algorithm, which I think is a good idea.

share|improve this answer
1  
This could be improved with TryGetValue so you only need to do the lookup once. –  pstrjds May 24 '12 at 17:33
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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