Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
{
    [DataMember]
    public decimal ListPrice { get; set; }

    [DataMember]
    public decimal Contribution
    {
        get
        {
            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
2  
I'm not sure if decimal is actually going to help you when the source data is in floats and doubles. –  svick Apr 23 at 11:42
add comment

1 Answer 1

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. –  Chuck Apr 23 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) –  Chuck Apr 23 at 11:26
1  
@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 Brizhak Apr 23 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. –  Chuck Apr 23 at 13:03
2  
@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 Brizhak Apr 23 at 13:15
show 1 more comment

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.