Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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 = "€";
    }
}
share|improve this question
    
I'm just wondering if what I've done below is correct. Is it doing what it's supposed to be doing? If it's not, it's considered broken. If you don't know, test it first. – Mast Jan 4 at 10:02
    
@Mast It is working, I mean correct in terms of design. – Yassin Hajaj Jan 4 at 10:02
    
@Mast, Coming from Java, I'm used to call constructors within other constructors but it is apparently impossible in C# – Yassin Hajaj Jan 4 at 10:03
    
it is possible. use ctor(x, y) : ctor(x) { this.y = y; } – Oguz Ozgul Jan 4 at 10:04
1  
your code does not behave as one would expect if monnaie is , e.g., GBP. It could for example throw an exception indicating the parameter is not supported. – njzk2 Jan 4 at 15:40
up vote 18 down vote accepted

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;
    }
}
share|improve this answer
1  
monnaie has the default value "€". Please check the field initialization of monnaie – Oguz Ozgul Jan 4 at 10:09
    
@Erik, how do you set the currency to Yen? monnaie is private and the only assignment to it is in the constructor if the monnaie parameter is passed in as $ ? – Oguz Ozgul Jan 5 at 8:09
    
You're right I read that wrong on my phone – Erik Jan 5 at 15:51

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;
        }
    }
    
share|improve this answer
    
I would use a set (HashSet<T>?) instead of the List, as the order is not important, and it shouldn't contain duplicates. – Juri Robl Jan 4 at 12:39
3  
Good work on your first answer! – Mast Jan 4 at 12:47
    
@JuriRobl I had considered a HashSet, but decided against it as there aren't likely to be enough (very short) strings to warrant calculating hash values. Logically, though, it is a set, not a list, so you're probably right. – Philip C Jan 4 at 13:28
    
Wow, thank you very much for the help. It is indeed very helpful and I learned a lot too. Thanks again ! – Yassin Hajaj Jan 4 at 23:04

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)
{ 
}
share|improve this answer

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.

share|improve this answer
1  
Thank you very much for the review but hence this is some school project, I had to use French. I usually also use English for all of my code. But that's indeed a nice review :) – Yassin Hajaj Jan 4 at 16:09
1  
@YassinHajaj You're welcome. I'm glad you liked my review. Since C# isn't a language that I'm 100% confortable with, I can't give you a very complete review. But I saw these issues and this was too much information for a single comment. The one you should focus the most is on the documentation. It literally takes 1 minute to write it, and Visual Studio automatically auto-completes almost everything. It's super easy and everybody that looks at your code, can have a clear explanation on what it does. Even when using, you can browser the multiple parameters and any info about any method. – Ismael Miguel Jan 4 at 17:39

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");
      }
   }
}
share|improve this answer
1  
Actually @Philip C mentioned default parameters briefly. It was his 3rd code block. – Rick Davin Jan 4 at 21:14

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.

share|improve this answer
    
Upvoted for pointing out the floating point issue, but int is overkill IMO; the ideal type to use for money is decimal (see here. – Mat's Mug Jan 5 at 5:15
    
Just wanted to mention it for completeness – henje Jan 5 at 9:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.