snippetcsharpMinor
How to refactor multiple if statements?
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;
```
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.
You are using the exception as a way to break out of the for loop. This is exactly what the
In addition, you are throwing the base class
When you throw an exception in your code, it should be a sub-class of
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.