How to shorten the following code

Question

1.00/5 (4 votes)

See more:

C#

public bool ValidCompulsoryFields()
    {
        if (ItemTypeName.Text == string.Empty || ItemTypeName.Text == StringConstants.select ||
                    ItemTypeName.SelectedValue.ToString() == StringConstants.select)
        {
            this.Master.LblError.Text= ErrorConstants.selectItemType;
            this.Master.LblError.ForeColor = Color.Red;
            ItemTypeName.Focus();
            return false;
        }

        if (ItemCategoryName.Text == string.Empty || ItemCategoryName.Text == StringConstants.select ||
                    ItemCategoryName.SelectedValue.ToString() == StringConstants.select)
        {
            this.Master.LblError.Text=ErrorConstants.selectItemCat;
            this.Master.LblError.ForeColor = Color.Red;
            ItemCategoryName.Focus();
            return false;
        }

        if (ManufacturerName.Text == string.Empty || ManufacturerName.Text == StringConstants.select ||
               ManufacturerName.SelectedValue.ToString() == StringConstants.select)
        {
            this.Master.LblError.Text=ErrorConstants.selectMake;
            this.Master.LblError.ForeColor = Color.Red;
            ManufacturerName.Focus();
            return false;
        }

        if (PackagingByName.Text == string.Empty || PackagingByName.Text == StringConstants.select ||
               PackagingByName.SelectedValue.ToString() == StringConstants.select)
        {
           this.Master.LblError.Text=ErrorConstants.selectPackage;
           this.Master.LblError.ForeColor = Color.Red;
                
PackagingByName.Focus();
           return false;
        }
        else
            return true;

Posted 17-Apr-11 21:56pm

durgeshtupkar10

Add a Solution

3 solutions

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

OriginalGriff · Accepted Answer · 2011-04-17T22:03:00

Assuming that "ItemTypeName", "ItemCategoryName" and so forth are all text boxs or similar, refactor it to a method:

private bool CheckValidity(TextBox tb, ErrorConstant fail)
   {
   if (tb.Text == string.Empty || tb.Text == StringConstants.select ||
               tb.SelectedValue.ToString() == StringConstants.select)
       {
       this.Master.LblError.Text= fail;
       this.Master.LblError.ForeColor = Color.Red;
       tb.Focus();
       return false;
       }
   return true;
   }

Then just call it repeatedly:

return CheckValidity(ItemTypeName,ErrorConstants.selectItemType) &&
       CheckValidity(ItemCategoryName,ErrorConstants.selectItemCat) &&
       CheckValidity(ManufacturerName,ErrorConstants.selectMake) &&
       CheckValidity(PackagingByName,ErrorConstants.selectPackage);

"But here, I've Four different textboxes.
Then how can I take them in one 'If Statement'?

actually i'm having 4 combo boxes not textboxes"

TextBox/ComboBox - it makes no difference! Just change the parameter type.

When you do a statement such as

bool b = bool1 && bool2 && bool3;

it evaluates them in strict order: Bool1 first. If this is true, evaluate bool2. If it isn't true, go no further because the whole expression is false.
As soon as the compiler gets to the point where a single false is encountered, it stops processing.
So:

return CheckValidity(ItemTypeName,ErrorConstants.selectItemType) &&
       CheckValidity(ItemCategoryName,ErrorConstants.selectItemCat) &&
       CheckValidity(ManufacturerName,ErrorConstants.selectMake) &&
       CheckValidity(PackagingByName,ErrorConstants.selectPackage);

will return true if and only if all of your TextBoxes / ComboBoxes pass the validation. If any one of them fails, it will stop processing, and return false;

Sergey Alexandrovich Kryukov · Accepted Answer · 2011-04-17T22:13:00

It's too boring to organize a rat race between experts who can make the shortest code, but yes, you code is unreasonably long.

Remember, you goal is not to make the code shorted but to improve maintainability and readability. This goal would change the code in the same direction (making it shorter), but only to certain extent. Main principle here is D.R.Y. (Do not Repeat Yourself — http://en.wikipedia.org/wiki/Do_not_repeat_yourself[^]).

You do repeat. Look at Color.Red, same in all branches — carry it our of if blocks. Conditions are almost similar — find a common denominator. Create a predicate function checking the condition; the different part of condition will be a parameter: this is the Text of ether PackagingByName, ManufacturerName, ItemCategoryName or ItemTypeName. When you use a predicate, each condition will be one short line (predicate call with parameter). As to the predicates, you can try to use my trick — inner method in the anonymous forms; see my Tips/Trick article: Hide Ad-hoc Methods Inside the Calling Method’s Body[^] (some readers find my method to be not readable, but I think it is — you decide).

Another general advice: try to formulate the laces of logical checks informally and implement closer to this formulation.

—SA

Manfred Rudolf Bihy · Accepted Answer · 2011-04-17T22:13:00

One way for sure would be to factor out a function that does all this along the lines of

C#

public bool TestSetError(ComboBox control, String error)
{
    bool retVal = true;
    if (control.Text == string.Empty || control.Text == StringConstants.select ||
        control.SelectedValue.ToString() == StringConstants.select)
    {
        this.Master.LblError.Text= error;
        this.Master.LblError.ForeColor = Color.Red;
        control.Focus();
        retVal = false;
    }
 
}

return (TestSetError(ItemTypeName,     ErrorConstants.selectItemType) &&
        TestSetError(ItemCategoryName, ErrorConstants.selectItemCat) &&
        TestSetError(ManufacturerName, ErrorConstants.selectMake) &&
        TestSetError(PackagingByName,  ErrorConstants.selectPackage)
       );

How to shorten the following code

3 solutions

Solution 1

Solution 3

Solution 2

Add your solution here

Preview 0