3
\$\begingroup\$

There are 3 search fields, and at least one must be filled out. When the search button is clicked the search fields are checked, if none are filled in show the error window with the error message for this error. If at least one field is filled in get a code based on the search criteria. If the code was not successfully retrieved show same error window but with the error message for this error.

The code I have works fine for this except that it looks verbose and I am repeated the code in the else statements except for the unique error messages. How can this be improved?

private void searchButton_Click(object sender, RoutedEventArgs e)
{
    searchCriteria.Clear();
    searchCriteria.Add(townSearchTextbox.Text);
    searchCriteria.Add(countySearchTextbox.Text);
    searchCriteria.Add(postcodeSearchTextbox.Text);

    if (isValidSearchCriteria(searchCriteria))
    {
        formatSearchCriteria(searchCriteria);
        set.updateWOEID(searchCriteria);
        if (set.WOEID.Length > 0)
        {
            // continue
        }
        else
        {
            // display any errors in the error window
            Error errorWindow = new Error();
            errorWindow.Show();
            errorWindow.errorMessage.Text = "Error: Could not retrieve WOEID, please try again.";
        }
    }
    else
    {
        // display any errors in the error window
        Error errorWindow = new Error();
        errorWindow.Show();
        errorWindow.errorMessage.Text = "Error: Please provide one or more search criteria.";
    }
}
\$\endgroup\$

2 Answers 2

7
\$\begingroup\$

Create a method:

private void DisplayError(String errorMessage) 
{
    // display any errors in the error window
    Error errorWindow = new Error();
    errorWindow.Show();
    errorWindow.errorMessage.Text = errorMessage;
}

Then you can do

private void searchButton_Click(object sender, RoutedEventArgs e)
{
    searchCriteria.Clear();
    searchCriteria.Add(townSearchTextbox.Text);
    searchCriteria.Add(countySearchTextbox.Text);
    searchCriteria.Add(postcodeSearchTextbox.Text);

    if (isValidSearchCriteria(searchCriteria))
    {
        formatSearchCriteria(searchCriteria);
        set.updateWOEID(searchCriteria);
        if (set.WOEID.Length > 0)
        {
            // continue
        }
        else
        {
            DisplayError("Error: Could not retrieve WOEID, please try again.");
        }
    }
    else
    {
        DisplayError("Error: Please provide one or more search criteria.");
    }
}

Of course, the next step if you need it would be to use Exceptions instead. Yes, I know this is for Java, but it shows why.

\$\endgroup\$
3
\$\begingroup\$

Considering that this is CodeReview, I've got a couple of remarks:

  1. isValidSearchCriteria is a method and thus should be in PascalCase. Same for formatSearchCriteria.

  2. WOEID: Microsoft's rule is to use Pascal case or camel case for acronyms more than two characters long, and to avoid abbreviations in identifiers or parameter names (if you must use abbreviations, use camel case for abbreviations that consist of more than two characters).

  3. Is searchCriteria a global field in this class? If so, why do you pass it to formatSearchCriteria; can't that method not simply use the global field?

  4. townSearchTextbox, countySearchTextbox, postcodeSearchTextbox: TextBox is a compound word and the first letter of each subsequent concatenated word needs to be capitalized.

Also, @Zhuinden's solution can be made even neater by reducing the identation this way:

private void searchButton_Click(object sender, RoutedEventArgs e)
{
    searchCriteria.Clear();
    searchCriteria.Add(townSearchTextbox.Text);
    searchCriteria.Add(countySearchTextbox.Text);
    searchCriteria.Add(postcodeSearchTextbox.Text);

    if (!isValidSearchCriteria(searchCriteria))
    {
        DisplayError("Error: Please provide one or more search criteria.");
        return;
    }

    formatSearchCriteria(searchCriteria);
    set.updateWOEID(searchCriteria);
    if (set.WOEID.Length <= 0)
    {
        DisplayError("Error: Could not retrieve WOEID, please try again.");
        return;
    }

    // apply your logic here
}
\$\endgroup\$
1
  • \$\begingroup\$ A great set of feedback thank you, I'm learning a lot! \$\endgroup\$
    – crmepham
    Commented Jan 30, 2015 at 14:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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