How to refactor multiple if statements?

Question

The code I want to refactor look as following:

public bool CheckPay(Int64 orderID)
{
    vpc_OrderInfo = orderID;
    RequestParams =
        string.Format(
            "vpc_Version={0}&vpc_Command={1}&vpc_AccessCode={2}&vpc_MerchTxnRef={3}&vpc_Merchant={4}&vpc_User={5}&vpc_Password={6}",
            vpc_Version, vpc_Command, vpc_AccessCode, vpc_MerchTxnRef, vpc_Merchant, vpc_User, vpc_Password);

    string[] response = SendRequest(vpc_VirtualPaymentClientURL, RequestParams).Split('&');
    bool ResponseCode = false;
    bool exist = false;
    Total = 0;
    TransactionDate = DateTime.Now;
    TransactionId = CardNo = vpc_Message = string.Empty;

    var query = new StringBuilder();
    try
    {
        foreach (string s in response)
        {
            if (string.IsNullOrWhiteSpace(s))
                throw new Exception(string.Format("Message='{0}' ", s));

            if (s.IndexOf("vpc_SecureHash") == -1)
            {
                string vpcMessage = s.Split('=')[1];
                query.AppendFormat("{0}-{1}|", s.Split('=')[0], vpcMessage);

                if (s.IndexOf("vpc_DRExists") > -1)
                {
                    exist = vpcMessage == "Y";
                }
                if (s.IndexOf("vpc_TxnResponseCode") > -1)
                {
                    ResponseCode = vpcMessage == "0";
                }
                if (s.IndexOf("vpc_Amount") > -1)
                {
                    Total =
                        double.Parse(vpcMessage.Length > 2
                                            ? vpcMessage.Insert(vpcMessage.Length - 2, ",")
                                            : "0," + vpcMessage);
                }
                if (s.IndexOf("vpc_BatchNo") > -1)
                {
                    TransactionDate = DateTime.Parse(vpcMessage.Insert(6, "-").Insert(4, "-"));
                }
                if (s.IndexOf("vpc_TransactionNo") > -1)
                {
                    TransactionId = vpcMessage;
                }
                if (s.IndexOf("vpc_CardNum") > -1)
                {
                    CardNo = vpcMessage;
                }
                if (s.IndexOf("vpc_Message") > -1)
                {
                    vpc_Message = vpcMessage;
                }
            }

            serviceLog.InfoFormat("Message: " + s);
        }
    }
    catch (Exception ex)
    {
        serviceLog.Error("fatal error", ex);
    }
    return exist && ResponseCode;
}

I'm not sure if I can get rid of these multiple if statements.

The "exist" and "ResponseCode" are boolean values and I need them to return bool type from method. Other values are class properties.

Answer 1

You could convert the if statements to a switch. The value being checked is the same in each if and would only be true in one instance. This should make the code easier to read.

Also, the query variable is being assigned, but not being used anywhere in the function so this can be removed.

foreach (string s in response)
{
    if (string.IsNullOrWhiteSpace(s))
        throw new Exception(string.Format("Message='{0}' ", s));

    if (s.IndexOf("vpc_SecureHash") == -1)
    {
        string vpcMessage = s.Split('=')[1];

        switch (s.Split('=')[0])
        {
            case "vpc_DRExists":
                exist = vpcMessage == "Y";
                break;

            case "vpc_TxnResponseCode":
                ResponseCode = vpcMessage == "0";
                break;

            case "vpc_Amount":
                Total = double.Parse(vpcMessage.Length > 2
                                        ? vpcMessage.Insert(vpcMessage.Length - 2, ",")
                                        : "0," + vpcMessage);
                break;

            case "vpc_BatchNo":
                TransactionDate = DateTime.Parse(vpcMessage.Insert(6, "-").Insert(4, "-"));
                break;

            case "vpc_TransactionNo":
                TransactionId = vpcMessage;
                break;

            case "vpc_CardNum":
                CardNo = vpcMessage;
                break;

            case "vpc_Message":
                vpc_Message = vpcMessage;
                break;
        }
    }

    serviceLog.InfoFormat("Message: " + s);
}

As a further consideration the various cases can be converted to const values.

// Update

outside the function - in class level

private const string RESPONSE_SECURE_HASH = "vpc_SecureHash"; // Name should be descriptive
private const string RESPONSE_DRExists = "vpc_DRExists";

and so on...

Then update the switch statement to be

case RESPONSE_DRExists :

see: https://stackoverflow.com/questions/9501893/using-a-lot-of-hardcoded-strings-in-code

Answer 2

This isn't related to your question, but you are not using exceptions properly.

try
{
    foreach (string s in response)
    {
        if (string.IsNullOrWhiteSpace(s))
            throw new Exception(string.Format("Message='{0}' ", s));

        //...
    }
}
catch (Exception ex)
{
    serviceLog.Error("fatal error", ex);
}

You are using the exception as a way to break out of the for loop. This is exactly what the break statement is for. Instead, you create a new Exception, which must populate the stack trace, and then the code starts to unwind the stack looking for the closest matching catch. This is an expensive process for something that is very basic. This could be replaced with:

foreach (string s in response)
{
    if (string.IsNullOrWhiteSpace(s))
    {
        serviceLog.Error("fatal error", string.Format("Message='{0}' ", s));
        break;
    }

    //...
}

In addition, you are throwing the base class Exception. This means that the most specific catch you can write is for the base class. This will catch all exceptions, including things like StackOverflowException and OutOfMemoryException. Your code will then try to continue on with no knowledge that these much more serious exceptions were swallowed up into a log message.

When you throw an exception in your code, it should be a sub-class of Exception that is as specific as you can be. This may be a predefined exception of an exception class you explicitly declare. By doing this, you can use the type system to handle different types of exceptions differently and not catch the exceptions you don't mean to handle.

Answer 3

1. Instead of using IndexOf and comparing the returned value to something, simply use the Contains method on string objects.

   if (!s.Contains("vpc_SecureHash")) { }
   
   if (s.Contains("vpc_DRExists")) { }
   

2. Your If-chain should be an if-else chain, because if one of those is true, then there is no reason to check the next condition. This behavior is easily emulated also in a switch statement