Mark Needham

Thoughts on Software Development

Coding: Reduce fields, delay calculations

with 13 comments

A pattern in code which I’ve noticed quite frequently lately is that of executing calculations in the constructor of an object and then storing the result in a field on the object.

From the small amount of experience I have playing around with functional languages I have come across the idea of lazy evaluation of functions quite frequently and I think it’s something that we can apply in object oriented languages as well.

When we store a value of a calculation in a field we are opening up the ability for that value to be changed before we use it.

We can certainly reduce/remove the chance of that happening by making fields read only or final but as Dhanji points out in his InfoQ article, if we are storing reference objects there is still a chance they could be mutated before we use them.

Even if we can manage to work our way around that problem I still feel that we increase the complexity of classes by having more fields as well as decreasing the readability of the code in the constructor since we now have all this extra information standing in our way when perhaps we don’t even care about it at all since it won’t be used until later on anyway.

I’m not sure if it’s a given but I never expect calculations to be done in the constructors of objects when I create them – at most I would expect the fields I pass in to be stored but any more than that is surprising and I think it’s good to avoid surprises if we can!

I don’t think the automated properties in C# 3.0 have really helped much and code like this is quite common:

public class SomeObject
{
	public SomeObject(Dependency1 dependency1, Dependency2 dependency2)
	{
		Field1 = dependency1.Calculation1();
		Field2 = dependency1.Calculation2();
		Field3 = dependency2.Calculation1();
		Field4 = dependency2.Calculation2();
		// and so on
	}
 
	public decimal Field1 { get; set; }
	public decimal Field2 { get; set; }
	public decimal Field3 { get; set; }
	public decimal Field4 { get; set; }
}

This one is relatively easy to simplify since we just need to store ‘dependency1′ and ‘dependency2′ and then delegate if the properties are called:

public class SomeObject
{
	private readonly Dependency1 dependency1;
	private readonly Dependency2 dependency2
 
	public SomeObject(Dependency1 dependency1, Dependency2 dependency2)
	{
		this.dependency1 = dependency1;
		this.dependency2 = dependency2;
		// and so on
	}
 
	public decimal Field1 { get { return dependency1.Calculation1(); }
	public decimal Field2 { get { return dependency1.Calculation2(); }
	public decimal Field3 { get { return dependency2.Calculation1(); }
	public decimal Field4 { get { return dependency2.Calculation2(); }
}

Now when we look at the constructor we can see that ‘SomeObject’ depends on those 2 classes and when we want to see what a call to ‘Field1′ does we can see straight away instead of having to scroll back up to the constructor to check.

In a way we have tightened the coupling between ‘SomeObject’ and ‘Dependency1′ and ‘Dependency2′ by storing those in fields and I was pondering the wisdom of doing this sort of thing in a previous post but I think I prefer to take this coupling over the alternative choice.

The choice isn’t quite as clear cut if we are getting a value from a dependency and then performing a calculation on that value.

For example:

public class SomeObject
{
	public SomeObject(Dependency1 dependency1)
	{
		Result = CalculateValueFrom(dependency1.OtherDependency);
	}
 
	public decimal Result { get; set; }
}

In this case I would still favour storing ‘Dependency1′ and then executing that calculation later although a law of demeter violation is more often than not staring us in the face and perhaps this isn’t the right class to be performing this calculation.

Be Sociable, Share!

Written by Mark Needham

September 2nd, 2009 at 11:52 pm

Posted in Coding

Tagged with

  • bogdan

    It definitely depends on what “Calculation1″ actually does. If it always returns the same result it makes sense to save it in a field instead of doing the calculation thousands of times during the life of the app.

  • http://www.markhneedham.com Mark Needham

    @bogdan that’s a good point didn’t think of that.

    When would you save it to the field though in the constructor?

    Or would you change the method to read something like this:

    public decimal Field1
    {
    get
    {
    if(calculation1 == null)
    {
    calculation1 = dependency1.Calculation1();
    }}
    return calculation1;
    }
    }

  • Alex O.

    Good post, thanks.

    One point – if there is a calculation involved on a dependency object, fields are probably not the right vehicle anyway. Fields are perceived as “data buckets”, not “workhorses” and therefore programmers frequently use fields as loop invariants (e.g. list.Count) or tokens. Invoking a calculation (that, mind you, may potentially call into a remote service, since it all depends on the dependency object’s implementation) would become a major performance bottleneck.
    A method would be a better alternative in this case.

  • Pingback: Reflective Perspective - Chris Alcock » The Morning Brew #426

  • Mike King

    There’s a nice combination of the two as well. Avoid the up front work of the constructor oriented approach, especially since it’s possible those values will never be needed, and the performance impact of repeating the same calculations over and over if that Property is used a lot and the value doesn’t change. Calculate it on demand but cache the value in a field, use a WeakReference if memory is a concern.

  • http://intwoplacesatonce.com Dave Cameron

    @bogdan, @mark, @mike

    I have been misled by assuming a repeated calculation will take more time than storing a calculated value in a field. Memory is not uniformly speedy. Even if a calculation always returns the same value, it can be faster to repeat a calculation thousands of times rather than fetching it from memory. It depends on caching, CLR behaviour, memory layout and so many other factors that predicting it ahead of time is quite difficult.

    At the same time, predictions of some accuracy can be made by looking at the amount of code involved. If a calculation is a whole class on its own with hundreds of lines, that could make sense to save in a field. If the calculation is only a handful of operations, like adding and multiplying two numbers, it may not make sense.

    I try to strive for code that only expresses the functional goal. Afterwards, if necessary, optimizations like caching calculations in fields can be made.

    In the specific example Mark is discussing, SomeObject is intended to be an adapter or aggregate that exposes several information sources to a consumer. It’s not intended to be an information holder for the information. And, there is already caching underneath the Dependency1 and Dependency2 classes, as they do abstract away some network calls.

  • http://intwoplacesatonce.com Dave Cameron

    @Mark

    In what way does storing the object increase the coupling? In both cases, SomeObject knows about Dependency1 and calls one of its methods. The difference is in when it is called, but not what is called.

    There is some run-time relationship difference, since the reference to Dependency1 means it will be garbage collected sometime after SomeObject is collected. Before, it may have been collected after SomeObject’s constructor ran. This isn’t what I normally think of as coupling though.

    @bogdan

    Thinking about it a bit more, your comment shows a type of coupling between the objects none of us have called out yet. By requiring SomeObject to know that Dependency1.Calculation1() always returns the same value, the version which stores the value in a field creates an implicit coupling between SomeObject and Dependency1. SomeObject needs to know that Calculation1() is implemented in such a way that the value does not change on repeated calls.

  • http://intwoplacesatonce.com Dave Cameron

    Very much agree on the Law of Demeter violation. From the example, CalculateValueFrom() really looks like it wants to live on dependency1.

  • http://www.markhneedham.com Mark Needham

    @Dave Didn’t see your second reply until today! Coupling wise I was thinking that if you stored it that meant a greater coupling but I think you’re right it knows about the Dependency so there’s no difference.

  • Pingback: Coding: An abstract class/ASP.NET MVC dilemma at Mark Needham

  • Pingback: Coding: Watch out for mutable code at Mark Needham

  • Pingback: Testability: How much logic belongs in the constructor? « Alexandre Gazola

  • Pingback: Scala: val, lazy val and def at Mark Needham