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

Question

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 = "€";
    }
}

Answer 1

You can invoke constructor overloads from each other as such:

public Prix(double valeur, string monnaie)

calls

public Prix(double valeur)

by:

public Prix(double valeur, string monnaie) : this(valeur)

Therefore,

You can simplify the exact same behaviour at least in terms of the final state as follows:

class Prix
{
    double valeur;
    string monnaie = "€";

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

    public Prix(double valeur)
    {
        this.valeur = valeur;
    }
}

EDIT 1:

You also don't have to check and assign moannie if it is passed in as euro because it already has the value euro:

class Prix
{
    double valeur;
    string monnaie = "€";

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

    public Prix(double valeur)
    {
        this.valeur = valeur;
    }
}

Answer 2

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<string> monnaieValid = new HashSet<string>
      {
          "$" // , "£", "¥", "฿", "₩"
      };
  
      ...
  
      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<T> 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<string> monnaieValid = new HashSet<string>
      {
          "$"
      };
  
      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;
      }
  }
  

Answer 3

Nobody mentioned it, but the idiomatic way of localizing currencies is to use System.Globalization.CultureInfo as a format provider: CultureInfo contains everything any code would ever need to know about a currency.

And... double is a floating-point data type, which is a very bad idea to use for anything related to money. Use decimal for currencies, it's still a floating-point data type, but its rounding errors will occur far beyond anywhere you'd care for any amount of money.

Hence, I'd expect a constructor signature like this:

public Prix(decimal valeur, CultureInfo culture)

And then you could be overriding ToString like this for example:

public override string ToString()
{
    return string.Format(_culture, "{0:C}", _valeur);
}

This way when the type is initialized with an "en-US" culture, the output would be in the shape of "$0.00".

I really don't like the idea of making it default to any specific culture - this would be less surprising:

public Prix(decimal valeur) : this(valeur, CultureInfo.InvariantCulture)
{ 
}

Answer 4

I know this is not the review you are looking for, but it must be said.

Please, avoid names in other languages.

It is really tempting to write variable and property names in other languages. But please, avoid it.

I am Portuguese, and can understand some of your code. Now imagine I want to change it, and will change it to my way.

Here is your original code with a tiny change to allow pounds(£), with a bit of Portuguese in it:

class Prix
{
    double valeur;
    string moeda;

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

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

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

"Moeda" means "coin" or "currency" in Portuguese. Is that what monnaie? Does it mean "currency"? I have no idea, but I assume it does.

Now, look at that code. What a beautiful mess! You can't understand what's there, I can't understand what's there... Nobody can understand it!

---

Bottom line is: Always use English names for your code. This avoids a huge linguistic bareer and helps to keep the code clean and easy-to-understand by everybody, specially those who don't speak French.

currency and value would be perfect names, if that is what valeur and monnaie mean...

---

Also, there's no documentation on your code. Not even a single comment, for non-French speakers. And even for French speakers!

How will I know what can I pass to the constructor if there's no documentation? How will I know that the 2nd parameter is optional? How would I know that it expects a double instead of a float?

If you use this in a project, 3 months from now, you will ask "What in the name of God this needs to run!?".

So, please, add some documentation to it. Visual Studio makes it easy to document your code. Just start by writting ///<sumary> and it will almost complete everything for you.

If you want more information, you can read @Shymep's answer on StackOverflow.

Answer 5

Default Parameters With all the extraneous stuff taken out, why not use default constructor with default parameter values? I haven't seen them mentioned yet.

class Prix
{
   double valeur;
   string monnaie;

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

With default parameter values you can use the first parameter alone (implicitly using the default)... or explicitly set the Symbol to "$" (or whatever you need it to be).

Pair this with some of the other options to verify you have a valid token:

class Prix
{
   static HashSet<string> monnaieValid = new HashSet<string>
   {
      "€", "$" //, "£", "¥", "฿", "₩"
   };
   double valeur;
   string monnaie;

   public Prix(double valeur, string monnaie = "€")
   {
      if (monnaieValid.Contains(monnaie))
      {
         valeur = valeur;
         monnaie = monnaie;
      }
      else
      {
         throw new ArgumentException(String.Format("{0} is not a valid option", monnaie), "moannaie");
      }
   }
}

Answer 6

Something I want to add to the other answers. Normally in finance money is saved in cents, etc., i.e. an integer.

class Price {

    int cents;
    string currency;

    public Price(int cents, string currency)
    {
        this.cents = cents
        ...
    }

    ...

}

This fits better because we don't have 1e-308 € intervals. Moreover floating point arithmetic has its own problems which you can avoid when using ints.