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 trying to improve my code’s "signal to noise ratio", hence would appreciate any tips on improving this, what appears to be smelly code. Perhaps there could also be potential performance improvements to be made here:

public class PricingInfoDto
    public decimal ListPrice { get; set; }

    public decimal Contribution
            var contributionAmount = (1 + TaxRate) * (float)ListPrice * (ContributionPercentage / 100);
            return Convert.ToDecimal(Math.Round(Math.Min(ContributionMaximumAmount, contributionAmount), 2));
        private set
            // For NHibernate Map

    public float TaxRate { get; set; }

    public double ContributionMaximumAmount { get; set; }

    public float ContributionPercentage { get; set; }

Unfortunately, I don’t have control over the types of properties, as the types of ContributionMaximumAmount & ContributionPercentage are enforced by the ADO.NET and the public DataMember is expected to be Decimal.

share|improve this question
I'm not sure if decimal is actually going to help you when the source data is in floats and doubles. – svick Apr 23 '14 at 11:42
up vote 4 down vote accepted

You are ignoring the whole point of decimals. Which is to avoid rounding errors from floating point caclulations.

You must cast TaxRate, ListPrice, ContributionPercentage, etc. to Decimal and only then do your calculations using Decimal values. Casting at some point is unavoidable, if you have no control over signatures. You can, however, implement a wrapper, which would do the casting and expose Decimal properties. That will make your code more pretty.

share|improve this answer
thanks, up-voted for decimal suggestion, please see the revised code. Any more comments appreciated. – Tsar Apr 23 '14 at 9:51
ah, changed my mind entirely in the end and created a SetContribution extension method for the PricingInfoDto. Now it (dto) looks nice & flat and I'm all green. Good time to break for lunch) – Tsar Apr 23 '14 at 11:26
@Chuck, i think it is a bad idea. You should not move core logic to extension methods. Initialization should be done in constructor. Or inside of the getter, thats fine too. But surely not inside some extension method noone knows about. – Nikita B Apr 23 '14 at 12:23
that's the intention: dto is part of the API Contract and nobody needs to know what the internal workings out are. This is a service response data DTO and the extension method is internal to the contract namespace. – Tsar Apr 23 '14 at 13:03
@Chuck, that is what private memebers are for. To hide the implemetation details. In my opinion "flat look" is not good enough reason to break the encapsulation (which you do by changing private to internal). It might not matter much in your case, but it is a bad practice nonetheless. – Nikita B Apr 23 '14 at 13:15

Your Answer


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.