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.

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.

share|improve this question
    
Why not convert to a switch statement? The data appears to be a single key-value pair. –  Kami Apr 7 at 10:19
1  
Why are you using IndexOf()? Shouldn't you just check s.Split('=')[0]? –  svick Apr 7 at 10:22
    
Also, how much more is left in the whole function... can you post the whole method? –  rolfl Apr 7 at 10:23
    
I've edited the original code and whole method is available now. –  tesicg Apr 7 at 10:28

3 Answers 3

up vote 5 down vote accepted

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: http://stackoverflow.com/questions/9501893/using-a-lot-of-hardcoded-strings-in-code

share|improve this answer
    
Why should various cases be converted to const values? –  tesicg Apr 7 at 11:42
2  
@tesicg It is a point for consideration, as the name of the constant is more descriptive then the string values. If this value is to be used at multiple locations, then it is easier to refractor the code at a later point. I have updated the answer to include a sample. –  Kami Apr 7 at 12:24

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.

share|improve this answer
2  
also, he is misusing the phrase "fatal error". A fatal error occurs when the entire program fails, but his catch block exists only in the CheckPay() function. It would be more accurate to print something along the lines of "error in CheckPay()" –  Red Alert Apr 7 at 18:08
  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

share|improve this answer

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.