HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcsharpMinor

How to refactor multiple if statements?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
statementsrefactormultiplehow

Problem

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;

Solution

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.

Code Snippets

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);
}
foreach (string s in response)
{
    if (string.IsNullOrWhiteSpace(s))
    {
        serviceLog.Error("fatal error", string.Format("Message='{0}' ", s));
        break;
    }

    //...
}

Context

StackExchange Code Review Q#46530, answer score: 7

Revisions (0)

No revisions yet.