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've got a class which describes and calculates centrifugal pump impeller properties (dimensions, flow speed etc. ). At the beginning i have created it in this way (since my class is quite large and calculations are complex i will simplify it using cylinder as example):

class Cylinder
{
    double height;
    double radius;

    //Properties rising event when value is changed
    //Constructors
    //Property changed event handler implementation

    public double FlatSideArea()
    {
        return Math.PI * radius;
    }
    public double Volume()
    {
         return FlatSideArea()*height;
    }
 }

So i have ended up with a lot of methods which calls each other, additionally number of unnecessary method calls is growing when i'm displaying results to the screen.

My last solution to this problem was creating a class in this way:

enum PropertyType
{
    Height,
    Radius,
    FlatSideArea,
    Volume
}
class Cylinder
{
    double height;
    double radius;
    double flatSideArea;
    double volume;

    public double Radius
    {
        get { return radius; }
        set
        {
            if (radius != value)
            {
                radius = value;
                RisePropertyChanged(PropertyType.Radius);
            }
        }
    }
    public double Height
    {
        get { return height; }
        set
        {
            if (height != value)
            {
                height = value;
                RisePropertyChanged(PropertyType.Height);
            }
        }
    }
    public double FlatSideArea
    {
        get { return flatSideArea; }
        private set
        {
            if (flatSideArea != value)
            {
                flatSideArea = value;
                RisePropertyChanged(PropertyType.FlatSideArea);
            }
        }
    }
    public double Volume
    {
        get { return volume; }
        private set
        {
            if (volume != value)
            {
                volume = value;
                RisePropertyChanged(PropertyType.Volume);
            }
        }
    }

    public Cylinder(double height, double radius)
    {
        this.height = height;
        this.radius = radius;
        this.flatSideArea = Equations.Area(radius);
        this.volume = Equations.Volume(flatSideArea, radius);
        PropertyChanged+=new EventHandler(Cylinder_PropertyChanged);
    }

    public delegate void EventHandler(Cylinder sender, PropertyType rodzajParametru);
    public event EventHandler PropertyChanged;

    private void RisePropertyChanged(PropertyType rodzajParametru)
    {
        if (PropertyChanged != null)
        {
            PropertyChanged(this, rodzajParametru);
        }
    }

    private void Cylinder_PropertyChanged(Cylinder sender, PropertyType propertyType)
    {
        switch (propertyType)
        {
            case PropertyType.Radius:
                FlatSideArea = Equations.Area(radius);
                break;
            case PropertyType.Height:
            case PropertyType.FlatSideArea:                    
                Volume = Equations.Volume(flatSideArea,height);
                break;
        }
    }
}

static class Equations
{
    public static double Area(double radius)
    {
        return Math.PI * Math.Pow(radius,2);
    }
    public static double Volume(double area, double height)
    {
        return area * height;
    }
}

Additional info:

  • My application is designing only impeller.
  • My class got almost 80 properties.
  • I have used separate static class with equations because i want to use some of the equation in other places.
  • User is allowed to change some of the properties and is provided "real time" with results

If example is still unclear let me know i will extend it.

Questions:

  • Is this a good solution or is there something better?
  • Since i know that i will be using only one type of event is it correct to use enum instead of EventArg?

Edit: After receiving very good answers i felt bad :) for not even following faq (pts .3 and 4 from on-topic questions) so i have extended my code. It is still not from my application but at least it is working now.

share|improve this question
add comment

4 Answers

up vote 6 down vote accepted

While commenting on your comment to Jesse's answer, I thought of a solution; Lazy<T>.

.NET 4 contains this new class, which for the purposes of most external consumers would have the following definition:

public class Lazy<T>
{
   private Func<T> func;
   private T val;
   private bool isCreated;

   public Lazy(Func<T> input)
   {
      func = input;
   }

   public bool IsValueCreated {get {return isCreated;}}

   public T Value 
   {
      get { 
         if(!isCreated)
         {
            val = func();
            isCreated = true;
         }
         return val;
      }
   }           
}

There's some additional code involved in the real implementation for thread-safety etc but this is the general idea (and it can be dropped into a .NET 3.5 solution if needed; .NET 2.0 doesn't have lambda statements or the Func<T> class but it's still possible to implement the concept). You can use it with an immutable class to provide for lazily-evaluated properties; the calculations for calculated fields are performed if and when they are needed, and are only performed on the first access of "Value", after which the result of the calculation is used without re-running it. The class doesn't HAVE to be immutable, but if consumers can change properties, the calculated properties that depend on the change must be recalculated when next accessed.

Here's your Cylinder class with the Lazy properties FlatArea and Volume:

class Cylinder
{
    readonly double height;
    readonly double radius;

    readonly Lazy<double> flatSideArea;
    readonly Lazy<double> volume;

    public Cylinder(double height, double radius)
    {
        this.height = height;
        this.radius = radius;

        this.flatSideArea = new Lazy<double>(()=>Math.PI * Math.Pow(radius, 2));
        this.volume = new Lazy<double>(()=>flatSideArea.Value * height);
    }

    public double FlatSideArea
    {
        get { return this.flatSideArea.Value; }
    }
    public double Volume
    {
         get { return this.volume.Value; }
    }
}

Usage:

//Only the user-defined properties are set here; no calculations.
Cylinder myCylinder = new Cylinder(20, 10);

//the FlatSideArea calculation is now performed:
var area = myCylinder.FlatSideArea;

//and now it's known, and doesn't have to be recalculated:
var vol = myCylinder.Volume;

//You don't have to access them in order of dependency either:
var vol2 = new Cylinder(10,20).Volume; //results in the calculation of FSA as well

So, the net result in your full situation of 80 properties is that, while re-instantiation to incorporate a change is still needed, calculation of dependent fields is not. If those 80 properties include a large number of purely calculated fields (and the calculation is not simply for sanity within a mutable class) you save a lot of work compared to "eager" calculation when a user is only interested in one or two calculations, or compared to a mutable class when needs to make several changes before being interested in the calculated fields.

If the class absolutely has to be mutable, then I would extract the instantiation of the "lazy" private fields into a method, "ClearCalculatedValues", and call it from the setters of the user-defined values if the new value differs from the old:

class Cylinder
{
    readonly double height;
    readonly double radius;

    readonly Lazy<double> flatSideArea;
    readonly Lazy<double> volume;

    public Cylinder(double height, double radius)
    {
        this.height = height;
        this.radius = radius;

        ClearLazyValues();
    }

    private void ClearLazyValues()
    {
        this.flatSideArea = new Lazy<double>(()=>Math.PI * Math.Pow(radius, 2));
        this.volume = new Lazy<double>(()=>flatSideArea * height);
    }

    public double FlatSideArea
    {
        get{return this.flatSideArea.Value;}
    }
    public double Volume
    {
         get{return this.volume.Value;}
    }

    public double Radius
    {
         get{return this.radius;}
         set{if(this.radius != value) {this.radius = value; ClearLazyValues();}
    }

    public double Height
    {
         get{return this.height;}
         set{if(this.height != value) {this.height = value; ClearLazyValues();}
    }
}

You can, if you want, limit the number of calculations that must be re-performed; for instance, a change to the Height property doesn't change the current value of FlatSideArea, so you can choose to only clear the Volume value. This will make that logic more complicated to code, but could save you some steps at runtime (which is a common tradeoff).

PS: BTW, if high precision is needed and you're dealing with cylinders (or other shapes) smaller in size than that of the known universe, I'd use decimal instead of double as it will reduce the amount of floating-point error inherent in storing base-10 numbers in base-2 scientific notation. A decimal can hold a number <= abs(+-7.9E28), which is suitable for most practical calculations outside astronomy and theoretical physics.

share|improve this answer
    
Nice catch on the formula for flatSideArea being wrong for a cylinder; I didn't even notice. And +1 for the decimal recommendation. –  Bill Barry Jul 9 '12 at 20:48
    
+1 and accepted for being first with Lazy<T> solution, decimal suggestion and catching my stupid mistake with area equation :). –  Archibald Jul 9 '12 at 21:45
add comment

I'm feeling like this isn't a good place to have properties changing. In other words, if your objects are indeed similar to a Cylinder, perhaps they should be immutable. That way, calculations can be a) done once at construction, b) done once, but lazily or c) done every time the property is accessed. But your mileage obviously will vary: if these are more of an entity object rather than an identity object, you'll have to do something different. But Cylinder seems like an identity to me. Example:

class Cylinder
{
    readonly double height;
    readonly double radius;

    readonly double flatSideArea;
    readonly double volume;

    public Cylinder(double height, double radius)
    {
        this.height = height;
        this.radius = radius;

        this.flatSideArea = Math.PI * radius;
        this.volume = this.flatSideArea * height;
    }

    public double FlatSideArea()
    {
        return this.flatSideArea;
    }
    public double Volume()
    {
         return this.volume;
    }
 }
share|improve this answer
    
Sorry if i haven't specified that before but user is allowed to change some of the properties and is provided with results in "real time". Creating new object each time user changes one property wouldn't become too slow? –  Archibald Jul 9 '12 at 19:32
    
I hate to break out the old canard, but "it depends" :) Since you mention that there's 80 properties, it likely could be expensive to recalculate all of them when a user changes one of them. Unless, of course, they will be requesting all those changed parts right afterwards. Then it's likely not that big of a deal. –  Jesse C. Slicer Jul 9 '12 at 19:38
    
How many of those 80 properties are "calculated" properties? In your simplified example of a Cylinder, asking for the cylinder's volume is trivial whether you do it in one method call or two. Method calls are cheap, relatively speaking; it's dereferencing (going out to the heap) that's more expensive. –  KeithS Jul 9 '12 at 20:02
    
@KeithS 72 are calculated properties most of them with a lot of math and one with simple iteration (no more then 20 recalculations) –  Archibald Jul 9 '12 at 20:24
    
So of the 80 properties, only 8 are user-definable? Check out my answer; I virtually guarantee it will solve your problem. –  KeithS Jul 9 '12 at 20:36
add comment

TLDR:

  • Implement an actual event if you want it to look like an event (don't make it look like a convention if you intend to not follow a convention).
  • Properly implement INotififyPropertyChanged if that is the event model you are trying for here.
  • Use properties without setters for those that don't make sense to edit.

ignoring immutability and such (like having a getter and no setter for FlatSideArea that computes on the fly)...

What you are basically implementing here is an internal usage of the INotifyPropertyChanged interface.

If you are going to implement that interface, I suggest you actually do so instead of only sorta partially doing so:

public class Cylinder : INotifyPropertyChanged {
    public class Properties {
        public const string Height = "Height";
        public const string Radius = "Radius";
        public const string FlatSideArea = "FlatSideArea";
        public const string Volume = "Volume";
    }
    double height;
    double radius;

    double flatSideArea;
    double volume;

    public event PropertyChangedEventHandler PropertyChanged;

    protected void SetProperty<T>(ref T field, T value, string name) {
        if (!EqualityComparer<T>.Default.Equals(field, value)) {
            field = value;
            var handler = PropertyChanged;
            if (handler != null) {
                handler(this, new PropertyChangedEventArgs(name));
            }
        }
    }

    public Cylinder() {
        PropertyChanged += OnPropertyChanged;
    }

    void OnPropertyChanged(object sender, PropertyChangedEventArgs propertyChangedEventArgs) {
        switch (propertyChangedEventArgs.PropertyName) {
        case Properties.Radius:
            FlatSideArea = Math.PI * Radius;
            goto propagateRadius; //note: break works here (due to confusing redirects)
        case Properties.Height:
        case Properties.FlatSideArea:
        propagateRadius:
            Volume = FlatSideArea * Height;
            break;
        }
    }

    public double Height {
        get { return height; }
        set { SetProperty(ref height, value, Properties.Height); }
    }

    public double Radius {
        get { return radius; }
        set { SetProperty(ref radius, value, Properties.Radius); }
    }

    public double FlatSideArea {
        get { return flatSideArea; }
        set { SetProperty(ref flatSideArea, value, Properties.FlatSideArea); }
    }

    public double Volume {
        get { return volume; }
        set { SetProperty(ref volume, value, Properties.Volume); }
    }
}

This implementation (whether or not you pull out the functions is irrelevant to this single use) will let you manage inheritance properly, but it does have some inefficiencies (setting radius raises the event 3 times, similar to how your implementation does). If you weren't concerned about inheritance you could simplify this by not bothering with the change event and modifying the private members outside of their own setters:

public class Cylinder {
    enum Properties {
        Height,
        Radius,
        FlatSideArea,
        Volume 
    }
    double height;
    double radius;

    double flatSideArea;
    double volume;


    void SetProperty<T>(ref T field, T value, Properties name) {
        if (!EqualityComparer<T>.Default.Equals(field, value)) {
            switch (name) {
            case Properties.Radius:
                flatSideArea = Math.PI * radius;
                goto propagateRadius; //note: break will not work here (using the fields means no recursion)
            case Properties.Height:
            case Properties.FlatSideArea:
            propagateRadius:
                volume = flatSideArea * height;
                break;
            }
        }
    }
    ... same as above for the public properties
}

One thing I would not do is name this method as if it were an event and then not use EventArgs based arguments. Conventions are good when everyone follows them, but when you don't but remain close enough that the code looks at a glance like you are then you have only made the future editor of your code think more.

Edit:

Using .NET 4, (still ignoring immutability) I would probably implement it like this:

public class Cylinder {
    double height;
    double radius;

    Lazy<double> flatSideArea;
    Lazy<double> volume;

    void SetProperty<T>(ref T field, T value) {
        if (!EqualityComparer<T>.Default.Equals(field, value)) {
            field = value;
            ResetComputedFields();
        }
    }

    void ResetComputedFields() {
        flatSideArea = new Lazy<double>(() => Math.PI * Radius);
        volume = new Lazy<double>(() => FlatSideArea * Height);
    }

    public Cylinder() {
        ResetComputedFields();
    }

    public double Height {
        get { return height; }
        set { SetProperty(ref height, value); }
    }

    public double Radius {
        get { return radius; }
        set { SetProperty(ref radius, value); }
    }

    public double FlatSideArea {
        get { return flatSideArea.Value; }
    }

    public double Volume {
        get { return volume.Value; }
    }
}
share|improve this answer
    
Copycat!! :P ;) –  KeithS Jul 9 '12 at 20:46
    
Done before seeing your answer and slightly better IMO (not resetting if you set new radius the same as old), and it doesn't have syntax errors :) –  Bill Barry Jul 9 '12 at 20:59
    
+1 for extended Lazy<T> solution and criticizing my method named like event but not really being one :). My first question here and I've got really hard time choosing which answer to accept i love SE sites :). –  Archibald Jul 9 '12 at 21:50
add comment

In addition to the other two answers, from Bill Barry and KeithS, which I really liked, I wanted to toss out some more code, because, well, CODE.

I happen to use a base class which implements some boilerplate INotifyPropertyChanged which allows my property setters to look like this:

public class Cylinder : PropertyChangedBase
{
    private double radius;

    //// Other stuff ...

    public double Radius
    {
        get
        {
            return this.radius;
        }

        set
        {
            this.SetProperty(ref this.radius, value, () => this.Radius);
        }
    }
}

Here's the class (note the SetProperty which is quite similar to the one shown):

public abstract class PropertyChangedBase : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    protected void RaisePropertyChanged(string propertyName)
    {
        var propertyChanged = this.PropertyChanged;

        if (propertyChanged != null)
        {
            propertyChanged(this, new PropertyChangedEventArgs(propertyName));
        }
    }

    protected bool SetProperty<T>(ref T backingField, T Value, Expression<Func<T>> propertyExpression)
    {
        var changed = !EqualityComparer<T>.Default.Equals(backingField, Value);

        if (changed)
        {
            backingField = Value;
            this.RaisePropertyChanged(ExtractPropertyName(propertyExpression));
        }

        return changed;
    }

    private static string ExtractPropertyName<T>(Expression<Func<T>> propertyExpression)
    {
        var memberExp = propertyExpression.Body as MemberExpression;

        if (memberExp == null)
        {
            throw new ArgumentException("Expression must be a MemberExpression.", "propertyExpression");
        }

        return memberExp.Member.Name;
    }
}
share|improve this answer
add 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.