C# code rewrite / redesign question

[moongirl129]

I have a bit of code here which looks really awful to me - what would be the best way to redesign/rewrite this?

public ImpactType ImpType
        {
            get 
            {
                if (costD)
                {
                    if (r1)
                    {
                        impType = ImpactType.D1;
                    }
                    else if (r2)
                    {
                        impType = ImpactType.D2;
                    }
                    else if (r3)
                    {
                        impType = ImpactType.D3;
                    }
                    else if (r4)
                    {
                        impType = ImpactType.D4;
                    }

                }
                else if (costC)
                {
                    if (r1)
                    {
                        impType = ImpactType.C1;
                    }
                    else if (r2)
                    {
                        impType = ImpactType.C2;
                    }
                    else if (r3)
                    {
                        impType = ImpactType.C3;
                    }
                    else if (r4)
                    {
                        impType = ImpactType.C4;
                    }
                }
                else if (costB)
                {
                    if (r1)
                    {
                        impType = ImpactType.B1;
                    }
                    else if (r2)
                    {
                        impType = ImpactType.B2;
                    }
                    else if (r3)
                    {
                        impType = ImpactType.B3;
                    }
                    else if (r4)
                    {
                        impType = ImpactType.B4;
                    }
                }
                else if (costA)
                {
                    if (r1)
                    {
                        impType = ImpactType.A1;
                    }
                    else if (r2)
                    {
                        impType = ImpactType.A2;
                    }
                    else if (r3)
                    {
                        impType = ImpactType.A3;
                    }
                    else if (r4)
                    {
                        impType = ImpactType.A4;
                    }
                }
                return impType; 
            }
        }

This is part of a class which relates to other properties as follows:

        public bool R1
        {
            get { return r1; }
            set { r1 = value; }
        }

        public bool CostA
        {
            get { return costA; }
            set { costA = value; }
        }

 

[stevexff]

All the booleans are a bit scary. Unless CostA-D and r1-4 are mutually exclusive, then you may not get the answer you are expecting. Would using [flags] on the enum help at all?

Steve

[small]"Every program can be reduced by one instruction, and every program has at least one bug. Therefore, any program can be reduced to one instruction which doesn't work." (Object:erlDesignPatterns)[/small]

 

[Kalisto]

Have a single method that takes Cost as a parameter.

Get that to return an integer.

Make ImpactType a bit array (It can still infact be an enum, just decorate it with the [Flags] attribute, and make sure each value is an exact power of 2 (0,1,2,4,8,16 etc)

Use the number returned from the method to shift a bit a number of places let in the array?

private int DoWork(r1)
{
  if (r1)
  {
     return 1;
  }
  ... 
  etc
}

elsewhere

if (costA)
  {
      int bitShift = DoWord(r1);
      //Cosntant really
      ImpactType startingPoint = 0;
      impType = startingPoint << bitShift;
  }

}

its kinda pseudo code, but hopefully you get the idea

k

 

[misterstick]

so, ImpactType is an enumeration? are the enumeration values arbitrary?

if not, try this:

public string CostType
{
 get
 {
  if ( costD ) return "D";
  if ( costC ) return "C";
  if ( costB ) return "B";
  if ( costA ) return "A";
  return string.Empty;
 }
}

public string RType
{
 get 
 {
  if ( r1 ) return "1";
  if ( r2 ) return "2";
  if ( r3 ) return "3";
  if ( r4 ) return "4";
  return string.Empty;
 }
}

public string RCost
{
 get
 {
  return CostType + RType;
 }
}

public ImpactType ImpType
{
 get
 {
  switch ( RCost )
  {
   case "A1": return ImpactType.A1;
   // ...
   default: return ImpactType.None;
  }
 }
}

mr s. <

 

[hilfy]

I would move it all into a method that takes both the Cost and the R values as parameters and returns the value for ImpType and then use Switch statements to make it more readable. Something like this (assuming parameters and outputs are strings and using the CostType and RType methods from misterstick's code...):

private string getImpType(string cost, string rType)
{
  string result = string.empty;
  switch(cost)
  {
    case "D":
      result = getDValue(rType);
      break;
    case "C":
      switch (rType)
      {
        case "1":
          result = ImpactType.C1;
          break;
        case "2":
          result = ImpactType.C2;
          break;
        case "3":
          result = ImpactType.C3;
          break;
        case "4":
          result = ImpactType.C4;
          break;
      } 
      break;
      <etc. for "B" and "A">
  }
}

private string getDValue(string rType)
{
  switch (rType)
  {
    case "1":
      result = ImpactType.D1;
      break;
    case "2":
      result = ImpactType.D2;
      break;
    case "3":
      result = ImpactType.D3;
      break;
    case "4":
      result = ImpactType.D4;
      break;
  }
}
<etc for getCValue, getBValue, and getAValue>

For these types of things I think that switch statements are much more readable than nested If statements and it also can help to break things down into multiple methods.

-Dell

A computer only does what you actually told it to do - not what you thought you told it to do.

 

[moongirl129]

Thanks for all the great tips people!! I have rewritten the code and it is so much easier to follow!