开发者

Refactoring big if else code

I have this really long if else code

 if (errorNumbers.Length == 6)
      {
        if (errorNumbers.Substring(0,4).Equals("1101") || errorNumbers.Substring(0,4).Equals("2121"))
        {
          retStr = "AutoRepickAfterPickError";
        }
        else if (errorNumbers.Substring(0, 4).Equals("开发者_JAVA百科1301") || errorNumbers.Substring(0, 4).Equals("2321"))
        {
          retStr = "AutoRepickAfterLAlignError";
        }
        else if (errorNumbers.Substring(0, 4).Equals("1401") || errorNumbers.Substring(0, 4).Equals("2221"))
        {
          retStr = "AutoRepickAfterCAlignError";
        }
        else if (errorNumbers.Substring(0, 4).Equals("1501") || errorNumbers.Substring(0, 4).Equals("2041"))
        {
          retStr = "AutoRepicksAfterManualRecovery";
        }

etc.....

How I can rewrite it to something more nice . Trying to learn here something new , and I am in .net 2.0. Thanks for help.


I´d map the error codes in a dictionary like this:

string errorCode = errorNumbers.Substring(0, 4);
Dictionary<string, string> map = new Dictionary<string,string>();
map.Add("1501", "AutoRepicksAfterManualRecovery");
map.Add("2041", "AutoRepicksAfterManualRecovery");
map.Add("1401", "AutoRepickAfterCAlignError");
map.Add("2221", "AutoRepickAfterCAlignError");
map.Add("1301", "AutoRepickAfterPickError");
map.Add("2121", "AutoRepickAfterPickError");
// etc

if(!map.ContainsKey(errorCode))
 throw new Exception("Invalid error code");

return map[errorCode];


That should be pretty straightforward

if (errorNumbers.Length == 6)
{
    string errNo = errorNumbers.Substring(0, 4);

    switch (errNo)
    {
        case "1101":
        case "2121":
            retStr = "AutoRepickAfterPickError";
            break;
        case "1301":
        case "2321":
            retStr = "AutoRepickAfterLAlignError";
            break;
        case "1401":
        case "2221":
            retStr = "AutoRepickAfterCAlignError";
            break;
        case "1501":
        case "2041":
            retStr = "AutoRepicksAfterManualRecovery";
            break;
    }
}

Also note that unlike in Java, in C# you can compare strings with ==.

"123" == "456" does the same thing as "123".Equals("456").


First off, DRY Principle - Don't Repeat Yourself. Assign the result of errorNumbers.Substring(0, 4) to a local variable:

string subNumbers = errorNumbers.SubString(0, 4);
if (subNumbers == "1401") // ...

Support for specific value cases (not ranges):

You can use a switch statement with strings:

switch(subNumbers)
{
    case "1401":
    case "2221":
      retStr = "AutoRepickAfterCAlignError";
      break;
    case "1501":
    case "2041":
      retStr = "AutoRepicksAfterManualRecovery";
      break;
    // etc
}

An alternative to a switch statement is creating a dictionary:

var selections = new Dictionary<string, string>()
{
    { "1401", "AutoRepickAfterCAlignError" },
    { "2221", "AutoRepickAfterCAlignError" },
    { "1501", "AutoRepicksAfterManualRecovery" },
    { "2041", "AutoRepicksAfterManualRecovery" },
    // etc
};

if (selections.ContainsKey(subNumbers))
    retStr = selections[subNumbers];

Ranges:

If you need to support ranges, you're basically going to have to stick with the if/else blocks, though. There are other options, but they tend to be too convoluted if you only have so many of these types of if/else blocks in your code.


You should initialize a dictionary with refferencies betwen errorCode and errorMessage

Dictionary<int, string> errorsCache = new Dictionary<int, string>()
{
  {1101,"AutoRepickAfterPickError"},
  {2121,"AutoRepickAfterPickError"},
  {1401,"AutoRepickAfterCAlignError"},
  ...
}

You can define GetErrorDescription method

public string GetErrorDescription(string errorNumbers)
{
  string retStr;
  if (errorNumbers.Length == 6)
  {
    int errorCode;  
    if(int.TryParse(errorNumbers.Substring(0,4), out errorCode))
    {
      errorsCache.TryGetValue(errorCode, out retStr);
    }
  }

  return retString;
  // or you can return empty string instead of null
  return retString ?? "";
}


First, store errorNumbers.Substring(0,4) in a local variable before the long if/else.

Then, you can eliminate the whole if/else using a map / dictionary from the expected error numbers to the appropriate return strings.


You could try this:

if (errorNumbers.Length == 6)
{

    //Makes it easier to change error codes, as theyre all in one place
    string[] AutoRepickAfterPickError = { "1101", "2121"};
    string[] AutoRepickAfterLAlignError = { "1301", "2321"};
    string[] AutoRepickAfterCAlignError = { "1401", "2221"};
    string[] AutoRepicksAfterManualRecovery = { "1501", "2041"};
    string subString = errorNumbers.Substring(0,4);

        if (AutoRepickAfterPickError.Contains(subString));
        {
          retStr = "AutoRepickAfterPickError";
        }
        else if (AutoRepickAfterLAlignError.Contains(subString))
        {
          retStr = "AutoRepickAfterLAlignError";
        }
        else if (AutoRepickAfterCAlignError.Contains(subString))
        {
          retStr = "AutoRepickAfterCAlignError";
        }
        else if (AutoRepicksAfterManualRecovery.Contains(subString))
        {
          retStr = "AutoRepicksAfterManualRecovery";
        }
}


What about Select-Case? :D

errorNumber = errorNumbers.Substring(0,4)
Select Case errorNumber 
   Case 1101,2121
      retStr = "AutoRepickAfterPickError";
   Case 1301,2321
      retStr = "AutoRepickAfterLAlignError";
   ...
End Select
0

上一篇:

下一篇:

精彩评论

暂无评论...
验证码 换一张
取 消

最新问答

问答排行榜