Constructors for a class to represent a price in € or $

I'm using a method to make initialize the attributes of my objects, and this method is called from both constructor.

What it does is, if no correct currency has been passed, choose € by default.

I'm just wondering if what I've done below is correct in terms of design.

class Prix
{
    double valeur;
    string monnaie;

    public Prix(double valeur, string monnaie)
    {
        if (monnaie.Equals("€") || monnaie.Equals("$"))
        {
            this.valeur = valeur;
            this.monnaie = monnaie;
        }
        else
        {
            defaultConstr(valeur);
        }
    }

    public Prix(double valeur)
    {
        defaultConstr(valeur);
    }

    private void defaultConstr(double valeur)
    {
        this.valeur = valeur;
        this.monnaie = "€";
    }
}

Starting from:

class Prix
{
    double valeur;
    string monnaie;

    public Prix(double valeur, string monnaie)
    {
        if (monnaie.Equals("€") || monnaie.Equals("$"))
        {
            this.valeur = valeur;
            this.monnaie = monnaie;
        }
        else
        {
            defaultConstr(valeur);
        }
    }

    public Prix(double valeur)
    {
        defaultConstr(valeur);
    }

    private void defaultConstr(double valeur)
    {
        this.valeur = valeur;
        this.monnaie = "€";
    }
}

-
When you have multiple constructors with similar parameters, it makes sense to keep the construction logic local to one constructor. One approach is to make one constructor call straight through to a second with any extra parameters.

public Prix(double valeur)
    : this(valeur, "€")
{
}

public Prix(double valeur, string monnaie)
{
    ...
}

Alternatively, supplying a default value for optional parameters is possible:

public Prix(double valeur, string monnaie = "€")
{
    ...
}

(see this question for information about the difference between the two)

-
Having construction logic in one place means that defaultConstr is only being called in one place. This method call can be inlined:

public Prix(double valeur, string monnaie)
{
    if (monnaie.Equals("€") || monnaie.Equals("$"))
    {
        this.valeur = valeur;
        this.monnaie = monnaie;
    }
    else
    {
        this.valeur = valeur;
        this.monnaie = "€";
    }
}

-
We're now setting valeur in both branches, so that can be factored out:

public Prix(double valeur, string monnaie)
{
    this.valeur = valeur;

    if (monnaie.Equals("€") || monnaie.Equals("$"))
    {
        this.monnaie = monnaie;
    }
    else
    {
        this.monnaie = "€";
    }
}

-
There is no need to compare against € if that will be assigned anyway:

public Prix(double valeur, string monnaie)
{
    this.valeur = valeur;

    if (monnaie.Equals("$"))
    {
        this.monnaie = monnaie;
    }
    else
    {
        this.monnaie = "€";
    }
}

-
Next, consider what happens if we have more valid currency symbols, we could end up with code like this:

if (monnaie.Equals("$") || monnaie.Equals("£") || monnaie.Equals("¥") || monnaie.Equals("฿") || monnaie.Equals("₩"))
{
    ...
}

This is getting quite ungainly - it's best to put this into a collection that can be checked against:

class Prix
{
    static HashSet monnaieValid = new HashSet
    {
        "$" // , "£", "¥", "฿", "₩"
    };

    ...

    public Prix(double valeur, string monnaie)
    {
        this.valeur = valeur;

        if (monnaieValid.Contains(monnaie))
        {
            this.monnaie = monnaie;
        }
        else
        {
            this.monnaie = "€";
        }
    }
}

Because the collection of currency symbols is logically a set (no duplicates, unordered), a HashSet is the appropriate choice for the collection. It is static because we don't need every Prix object to have its own HashSet - one shared copy is enough.

-
It is also advisable to avoid "magic strings" - create a constant for the default currency and use this instead of the literal string:

class Prix
{
    const string MonnaieDefault = "€";

    ...

    public Prix(double valeur) 
        : this(valeur, MonnaieDefault)
    {
    }

    public Prix(double valeur, string monnaie)
    {
        this.valeur = valeur;

        if (monnaieValid.Contains(monnaie))
        {
            this.monnaie = monnaie;
        }
        else
        {
            this.monnaie = MonnaieDefault;
        }
    }
}

-
I would also make a few other small changes to help readability (although this step may be subjective):

• Add access modifiers where it's currently implicit

• Make fields readonly wherever possible - here no fields are modified after they are initialised, so they can all be made readonly

• Use a ternary operator instead of the if statement

Finally, the class looks like this:

public class Prix
{
    private const string MonnaieDefault = "€";

    private static readonly HashSet monnaieValid = new HashSet
    {
        "$"
    };

    private readonly double valeur;
    private readonly string monnaie;

    public Prix(double valeur) 
        : this(valeur, MonnaieDefault)
    {
    }

    public Prix(double valeur, string monnaie)
    {
        this.valeur = valeur;
        this.monnaie = monnaieValid.Contains(monnaie) ? monnaie : MonnaieDefault;
    }
}

StackExchange Code Review Q#115784, answer score: 30