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.

I have below code in one function:

public void func()
{
    if (!string.IsNullOrEmpty(customerpay) && !string.IsNullOrEmpty(warrantypay) && !string.IsNullOrEmpty(maintanceplan))
    {
        if (customerpay == "Include" && warrantypay == "Include" && maintanceplan == "Include")
        {
            transactiontype = "," + "[Population].[Transaction Type Code].&[C^W^I]";
        }
        else if (customerpay == "Include" && warrantypay == "Include" && maintanceplan != "Include")
        {
            transactiontype = "," + "[Population].[Transaction Type Code].&[C^W]";
        }
        else if (customerpay == "Include" && warrantypay != "Include" && maintanceplan != "Include")
        {
            transactiontype = "," + "[Population].[Transaction Type Code].&[C]";
        }
        else if (customerpay != "Include" && warrantypay == "Include" && maintanceplan == "Include")
        {
            transactiontype = "," + "[Population].[Transaction Type Code].&[W^I]";
        }
        else if (customerpay != "Include" && warrantypay != "Include" && maintanceplan == "Include")
        {
            transactiontype = "," + "[Population].[Transaction Type Code].&[I]";
        }
        else if (customerpay != "Include" && warrantypay == "Include" && maintanceplan != "Include")
        {
            transactiontype = "," + "[Population].[Transaction Type Code].&[W]";
        }
        else if (customerpay == "Include" && warrantypay != "Include" && maintanceplan == "Include")
        {
            transactiontype = "," + "[Population].[Transaction Type Code].&[C^I]";
        }
    }
}

I need to simplify the above if-else C# code. Is there a way to do this?

share|improve this question
    
One thing you could do is have those (stringVar) == "Include" as boolean checks at the start of the if statement, like so: if(/*none are null*/) { bool hasCustomerPay = customerpay == "Include"; bool hasWarrentPay = warrentpay == "Include"; bool hasMaintancePlan = maintanceplan == "Include"; /*long if-else here*/ } –  Tory Jan 27 at 17:46
    
What is logic behind this code? A bet you have some rules to decide whether you add one or more 'lines' to transactiontype, and rule which you use for generation C^WI combinations –  Sergey Berezovskiy Jan 29 at 14:54
add comment

2 Answers

You could do it like this:

public void func()
{
    if (string.IsNullOrEmpty(customerpay) || string.IsNullOrEmpty(warrantypay) || string.IsNullOrEmpty(maintanceplan))
        return;

    if (customerpay != "Include" && warrantypay != "Include" && maintanceplan != "Include")
        return;

    List<char> parts = new List<char>();
    if (customerpay == "Include")
        parts.Add("C");
    if (warrantypay == "Include")
        parts.Add("W");
    if (maintanceplan == "Include")
        parts.Add("I");

    transactiontype = ",[Population].[Transaction Type Code].&[" + string.Join("^", parts) + "]";
}

First, we simply return if any of the strings is empty; by inverting the logic, we can reduce one indentation level that makes the code unecessarily complicated. We also return for the single case that isn’t handled in your original code: If all three strings are not "Include".

After that, it’s easy once you find out the pattern of your string. The only changing part of your result string is in the brackets at the end. There are three possible characters: C, W, and I. They appear if the corresponding string equals to "Include". The characters are separated by a ^. So we just check each condition alone, and collect the characters in a list. Then we join with the separation character and build the final result string.

share|improve this answer
    
+1 beat me to it :) Though, you could probably omit the IsNullOrEmpty checks since the inequality check takes care of it. –  Dan Lyons Jan 27 at 17:56
    
+1 for beating me to it also :) ... and for handling the unlikely case where none of the strings say "Include". –  Mat's Mug Jan 27 at 18:00
    
@DanLyons Good idea, but that is actually only half-true. When just one of the strings equals "Include", then the inequality check is skipped, but the two other variables could still be null ;) –  poke Jan 27 at 19:00
    
@poke thanks for your solution.i have modified question by added some more conditions. could you please check once again and please let me know how can i use this solution for that –  SivaRajini Jan 28 at 13:42
    
@SivaRajini Does the order of the resulting things matter? –  poke Jan 28 at 14:27
show 1 more comment

Well the first condition could be reversed, immediately reducing the nesting level:

if (string.IsNullOrEmpty(customerpay) || string.IsNullOrEmpty(warrantypay) || string.IsNullOrEmpty(maintanceplan))
{
   return;
}

// rest of conditions

This code looks like sample code, but I'll say it anyway: func is a bad name for any function, especially one that returns void. And to follow naming C# conventions it should be named Func - but that clashes with the framework's Func<T>, making it even more confusing. Give meaningful names!!

Back to the else if blocks..

This code builds a string. I think transactionType should be transactionTypeBuilder and be of type StringBuilder - don't concatenate strings like that; build them instead!

Moreover, each branch is really coming up with the very last part to be added to the string, so it would be easier to understand what the code does at a glance if you introduced a variable and changed the transactionType assignment to this:

transactionType = string.Format(",[Population].[Transaction Type Code].&[{0}]", code);

Where code is what each branch is trying to solve. This leaves you with this:

var code = string.Empty;
if (customerpay == "Include" && warrantypay == "Include" && maintanceplan == "Include")
{
    code = "C^W^I";
}
else if (customerpay == "Include" && warrantypay == "Include" && maintanceplan != "Include")
{
    code = "C^W";
}
else if (customerpay == "Include" && warrantypay != "Include" && maintanceplan != "Include")
{
    code = "C";
}
else if (customerpay != "Include" && warrantypay == "Include" && maintanceplan == "Include")
{
    code = "W^I";
}
else if (customerpay != "Include" && warrantypay != "Include" && maintanceplan == "Include")
{
    code = "I";
}
else if (customerpay != "Include" && warrantypay == "Include" && maintanceplan != "Include")
{
    code = W";
}
else if (customerpay == "Include" && warrantypay != "Include" && maintanceplan == "Include")
{
    code = "C^I";
}

Now each test seems to be crying for a bool. Consider this:

var code = string.Empty;
var includeCustomerPay = (customerPay == "Include");
var includeWarrantyPay = (warrantyPay == "Include");
var includeMaintenancePlan = (maintenancePlan == "Include");

if (includeCustomerPay && includeWarrantyPay && includeMaintancePlan)
{
    code = "C^W^I";
}
else if (includeCustomerPay && includeWarrantyPay && !includeMaintancePlan
{
    code = "C^W";
}
else if (includeCustomerPay && !includeWarrantyPay && !includeMaintancePlan)
{
    code = "C";
}
else if (!includeCustomerPay && includeWarrantyPay && includeMaintancePlan)
{
    code = "W^I";
}
else if (!includeCustomerPay && !includeWarrantyPay && includeMaintancePlan)
{
    code = "I";
}
else if (!includeCustomerPay && includeWarrantyPay && !includeMaintancePlan)
{
    code = "W";
}
else if (includeCustomerPay && !includeWarrantyPay && includeMaintancePlan)
{
    code = "C^I";
}

Isn't something starting to emerge? The "code" for each bool is always in the same order, and separated by a "^".

Therefore, the whole condition can be dropped, with the help of a bit of :

var codes = new[] 
              { 
                  includeCustomerPay ? "C" : string.Empty,
                  includeWarrantyPay ? "W" : string.Empty,
                  includeMaintenancePlan ? "I" : string.Empty
              };

var code = string.Join("^", codes.Where(c => !string.IsNullOrEmpty));
transactionType = string.Format(",[Population].[Transaction Type Code].&[{0}]", code);
share|improve this answer
    
thanks for your solution.i have modified question by added some more conditions. could you please check once again and please let me know how can i use this solution for that updated question –  SivaRajini Jan 28 at 13:43
3  
@SivaRajini you realize you've just invalidated both answers with that edit? I suggest you rollback that edit, rework your code per our answers (and accept either), and post another CR question with your updated code. More work, but more votes for everyone :) –  Mat's Mug Jan 28 at 14:19
    
Agreed; just starting a new answer with the full code would be fine too (i.e. without reworking it), as the situation changed quite a bit, and our solutions don’t really apply easily to the new thing. –  poke Jan 28 at 14:40
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.