Switch statement in property c sharp-CodePudding

I have created an enum for accessories and used the switch statement in the class property to determine the price depending on the accessories chosen. A class that has two constructors implements this enum, one of the constructors accepts types from this enum as arguments while the other does not. See codes below:

Expected Result: If an instance of the class is instatiated with a type of the enum, switch to enum type and return price if an instance of the class is instantiated without providing an enum type price should be zero.

Problem Statement: When an instance is initialized without providing aan enum type the swtich statement runs and breaks in the first case

using System;

///<summary>
/// specifies the accessories of a vehicle
///</summary>
public enum Accessories
{
    StereoSystem = 0,
    LeatherInterior = 1,
    StereoAndLeather = 2,
    ComputerNavigation = 3,
    StereoAndNavigation = 4,
    LeatherAndNavigation = 5,
    All = 6,
    None = 7
}

public SalesQuote(decimal vehicleSalePrice, decimal tradeInAmount,
                  decimal salesTaxRate, Accessories accessoriesChosen,
                  ExteriorFinish exteriorFinishChosen)
{
    if (vehicleSalePrice <= 0)
        throw new ArgumentOutOfRangeException("vehicleSalePrice", "The arguement cannot"  
            " be less than or equal to zero");
    if (tradeInAmount < 0 || salesTaxRate < 0)
    {
        string myValue = tradeInAmount < 0 ? "tradeInAmount" : "salesTaxRate";
        throw new ArgumentOutOfRangeException(myValue, 
            "The arguement cannot be less than zero");
    }
    if(salesTaxRate > 1)
        throw new ArgumentOutOfRangeException("salesTaxRate",
            "The arguement cannot be greater than 1");
    if((!(accessoriesChosen.GetType() == typeof(Accessories) ||!(Enum.IsDefined(typeof(Accessories),accessoriesChosen)) )
        ||((!(exteriorFinishChosen.GetType() == typeof(ExteriorFinish)) || !(Enum.IsDefined(typeof(ExteriorFinish),exteriorFinishChosen))))))
        throw new System.ComponentModel.InvalidEnumArgumentException
            ("The arguement is an invalid enumeration value");

    this.vehicleSalePrice = vehicleSalePrice;
    this.tradeInAmount = tradeInAmount;
    this.salesTaxRate = salesTaxRate;
    this.accessoriesChosen = accessoriesChosen;
    this.exteriorFinishChosen = exteriorFinishChosen;
}

/// Second Constructor

public SalesQuote(decimal vehicleSalePrice, decimal tradeInAmount, decimal salesTaxRate)
{
    if (vehicleSalePrice <= 0 || tradeInAmount < 0)
    {
        string myValue = vehicleSalePrice <= 0 ? "vehicleSalePrice" : "tradeInAmount";
        string completionString = tradeInAmount < 0 ? "0" : "or equal to zero";
        throw new ArgumentOutOfRangeException(myValue, "The arguement cannot be less than "  
        completionString);
    }
    if (salesTaxRate < 0 || salesTaxRate > 1)
    {
        string completionString = salesTaxRate < 0 ? "less than 0" : "greater than 1";
        throw new ArgumentOutOfRangeException("salesTaxRate", "The arguement cannot be "  
            completionString);
    }
    this.vehicleSalePrice = vehicleSalePrice;
    this.tradeInAmount = tradeInAmount;
    this.salesTaxRate = salesTaxRate;
}

/// cost property

public decimal AccessoryCost
{
   get 
    {
        decimal accessoryPrice = 0;
        switch (this.AccessoriesChosen)
        {
            case Accessories.StereoSystem:
                accessoryPrice = 505.05m;
                break;
            case Accessories.LeatherInterior:
                accessoryPrice = 1010.10m;
                break;
            case Accessories.ComputerNavigation:
                accessoryPrice = 1515.15m;
                break;
            case Accessories.StereoAndNavigation:
                decimal total = 505.05m   1515.15m;
                accessoryPrice = total;
                break;
            case Accessories.LeatherAndNavigation:
                total = 1010.10m   1515.15m;
                accessoryPrice = total;
                break;
            case Accessories.All:
                total = 1010.10m   1515.15m   505.05m;
                accessoryPrice = total;
                break;
            default:
                accessoryPrice = 0;
                break;
        }
        return accessoryPrice;
    }
}

Initialize with second constructor

SalesQuote mySalesQuote = new SalesQuote(500,200,.5);
decimal cost = mySalesQuote.AccessoryCost;
/// cost should be zero in this instance since no accessory was selected but I am getting 505.05

Please can someone let me know why this is and how to resolve this.

Thanks

I have tried using conditions in the different cases but it appears the type is implicitly casted to the Accessories type which I don't understand why.

case Accessories.StereoSystem:
        accessoryPrice = !Enum.IsDefined(typeof(Accessories),accessoriesChosen)  ?   0 : 505.05m;
       break;

CodePudding user response：

Your class design is a bit off. You have 2 different constructors:

SalesQuote(decimal vehicleSalePrice, decimal tradeInAmount, decimal salesTaxRate)

and

SalesQuote(decimal vehicleSalePrice, decimal tradeInAmount,
    decimal salesTaxRate, Accessories accessoriesChosen,
    ExteriorFinish exteriorFinishChosen)

It seems that one constructor is a more powerful constructor than the other one. In such a case, you can forward the arguments from the simple constructor to the powerful constructor and provide default arguments.

That way you'll get rid of a lot of duplicate code (which isn't 100% duplicate any more, because they diverged already).

How does it look like?

public SalesQuote(decimal vehicleSalePrice, decimal tradeInAmount, decimal salesTaxRate)
:this(vehicleSalePrice, tradeInAmount, salesTaxRate, Accessories.None, ExteriorFinish.None)
{

}

Notice the :this() call. This constructor calls the more powerful constructor.

Also note: since the more powerful constructor needs 5 arguments, you have to provide the default values for the parameters accessoriesChosen and exteriorFinishChosen.

See how this code sets the default value to Accessories.None which fixes your bug.

BTW, you may want to have a look into the Decorator design pattern. Your way of declaring accessories through an enum will not hold for long. More accessories will result in an explosion of combinations. Your code will need more if-statements and break often.

CodePudding user response：

Enum constants like None or Undefined should always have the value 0. This makes them automatically the default value.

Since the enum contains combinations of values, it should be a flags enum

[Flags]
public enum Accessories
{
    None = 0,
    StereoSystem = 1 << 0,
    LeatherInterior = 1 << 1,
    ComputerNavigation = 1 << 2,

    StereoAndLeather = StereoSystem | LeatherInterior,
    StereoAndNavigation = StereoSystem | ComputerNavigation,
    LeatherAndNavigation = LeatherInterior | ComputerNavigation,
    All = StereoSystem | LeatherInterior | ComputerNavigation
}

The unique values are powers of two (1, 2, 4). This allows combining them to form new values and also to test whether a flag is set

if (accessories.HasFlag(Accessories.StereoSystem)) {

}

This simplifies the cost calculation since you don't need to check all the combinations

public decimal AccessoryCost
{
    get {
        decimal accessoryPrice = 0;
        if (this.AccessoriesChosen.HasFlag(Accessories.StereoSystem)) {
            accessoryPrice  = 505.05m;
        }
        if (this.AccessoriesChosen.HasFlag(Accessories.LeatherInterior)) {
            accessoryPrice  = 1010.10m;
        }
        if (this.AccessoriesChosen.HasFlag(Accessories.ComputerNavigation)) {
            accessoryPrice  = 1515.15m;
        }
        return accessoryPrice;
    }
}

E.g., this

mySalesQuote.AccessoriesChosen = Accessories.StereoAndNavigation;
Console.WriteLine(mySalesQuote.AccessoryCost);
mySalesQuote.AccessoriesChosen = Accessories.All;
Console.WriteLine(mySalesQuote.AccessoryCost);

prints

2020.20
3030.30