Lots of constructor parameters - Is there a better way?

Question

public class HourlyForecastData
{
    public DateTime DateTime { get; private set; }
    public decimal TemperatureCelcius { get; private set; }
    public decimal DewPoint { get; private set; }
    public string Condition { get; private set; }
    public int ConditionCode { get; private set; }
    public int WindSpeed { get; private set; }
    public string WindDirection { get; private set; }
    public decimal WindDegrees { get; private set; }
    public int UltravioletIndex { get; private set; }
    public decimal Humidity { get; private set; }
    public decimal WindChill { get; private set; }
    public int HeatIndex { get; private set; }
    public decimal FeelsLike { get; private set; }
    public decimal Snow { get; private set; }

    public HourlyForecastData(DateTime dateTime, decimal temperatureCelcius, ...)
    {
        DateTime = dateTime;
        TemperatureCelcius = temperatureCelcius;
        //...set all the other properties via constructor
    }
}

I am trying to learn better software design and OOP. I'm creating a library that can access a weather service that replies with XML. There are a lot of different fields provided by the service, so I've created properties for each of the XML fields. However, it feels a bit messy to have that number of properties set via the constructor. I could omit the constructor and have public setters but I'm trying to make an immutable class.

I've looked around at different design patterns for this and there seems to be some "Builder" and "Factory" patterns. However, I'm struggling to understand how I would apply that to my code. Or should I be using something completely different to fill the properties in these objects?

All of the properties are required. I don't think there is any kind of default values, the web service documentation mentions nothing about that. — user9993, Mar 22 '15 at 06:24
Is immutability really that important? Looks like a simple DTO so I'd leave the properties public then let the XMLSerializer fill it automatically. — Pierre-Luc Pineault, Mar 22 '15 at 06:31
Try the "builder" pattern. But it will not solve your problem from the root — avim, Mar 22 '15 at 07:25

score 10 · Accepted Answer · answered Mar 22 '15 at 06:45

In this case composition might be a good fit. Especially since there are some parameters that belongs to specific categories.

For instance:

public int WindSpeed;
public string WindDirection;
public decimal WindDegrees;

Create a new object for them and then access the different values as:

weatherData.Wind.Speed;

and pass the new wind object to the constructor:

var wind = new Wind(xmlData.WindSpeed, xmlData.WindDirection, xmldata.WindDegrees);
var weatherReport = new WeatherReport(wind, /* .... */);

I would also introduce a few enums. Because as of now, the users of the weatherReport would for instance have to know which values the string WindDirection can have. If you convert the string to an enum instead it's a lot easier to use the different values.

A final note is that I typically only use constructors if the are some values that really have to be specified for the class to have a valid state. For instance, in your case the minimum valid state would be a date and the temperature? Then just put those in the constructor.

Composition is a good idea. It will introduce more complexity during the deserialization, but you end up with much cleaner business entities. — Pierre-Luc Pineault, Mar 22 '15 at 07:02
This is a very helpful answer, thank you. You said just put the minimum required in the constructor, but then how would I set the other properties if not through the constructor, through public setters? — user9993, Mar 22 '15 at 15:22
@user9993: If you do not want to use public setters you can use reflection `obj.GetType().GetProperty("PropertyName").SetValue(obj, value);` I think it's acceptable in classes which converts information from a DTO to an entity object. — jgauffin, Mar 24 '15 at 08:15

StuartLC · Answer 2 · 2019-12-24T07:35:42.840

Re Is there a better OOP approach?

A large number of properties on a class can often indicate a need for splitting the class up (the Single Responsibility Principle of SOLID).

e.g. It would appear that HourlyForecastData models Wind (speed and direction), Precipitation (Snow, Dew and Rain), and Temperature (Min, Max ...) These concerns can be split into separate classes, and then the HourlyForecastData would be a composition of the three.

Re : Builder Pattern

The Builder Pattern can be useful to ease the burden during construction of large (often immutable) classes or graphs, but would obviously require additional (mutable) Builder class(es) to build up the target class representation (i.e. HourlyForecastData) and eventually create it (viz, by constructing it immutably by passing it all parameters to the constructor). So it isn't less effort, if that is what you required by 'better', but this can certainly can be easier to read e.g.:

HourlyForecastData todaysForecast = new HourlyForecastDataBuilder()
   .WithBaseline(ObjectMother.WinterSnow) // Provide an archetype
   .WithPrecipitation(snow: 5, rain:1) // Dew defaults to 0
   .Build();

Baseline archetypes / object mothers would be useful if the weather patterns in an area were frequently stable and just required small adjustments. IMO builder pattern is most useful in Testing. I can't see an obvious fit in an Xml Serialization usage.

See also Named and Optional parameters

Re: Immutability

A private setter technically still allows mutability, although restricted within the class itself. C#6 and later supports getter-only auto properties which is the simplest form for implementing immutable properties

public class HourlyForecastData
{
    public DateTime DateTime { get; }
    ...

    public HourlyForecastData(DateTime dateTime, ...)
    {
        // Get only auto properties can only be set at construction time
        DateTime = dateTime;
        ...

Unrelated, but Scala offers an even more concise syntax than C# for defining immutable public properties on a class, by declaring them once in the (primary) constructor:

class HourlyForecastData(val temperature: Int, val station: String, ...) {
}

Without the need for any further property or backing fields, whilst expressing and enforcing immutability. However, the burden still remains on the caller to provide all the parameters (whether directly, or via Builder, etc).

Re : Xml If you are offering an API, I would suggest using WebAPI. Instead of building Xml serialization concerns into your DTO classes, I would suggest instead on relying on Content Negotiation. This will allow the caller to determine whether the data should be returned in Xml or JSON format.

* Note however that Xml Deserialization technologies often make use of reflection to populate DTO properties, which MAY require that the serializable properties have setters (even if private).

Casey · Answer 3 · 2015-03-22T06:39:02.440

One way is to use a struct and pass it in instead. It also makes using the class easier as you only need to declare the struct state variable, change whatever differs from the "default" then pass it in.

public struct HourlyForecastDataState
{
    public DateTime DateTime;
    public decimal TemperatureCelcius;
    public decimal DewPoint;
    public string Condition;
    public int ConditionCode;
    public int WindSpeed;
    public string WindDirection;
    public decimal WindDegrees;
    public int UltravioletIndex;
    public decimal Humidity;
    public decimal WindChill;
    public int HeatIndex;
    public decimal FeelsLike;
    public decimal Snow;
}

public class HourlyForecastData
{
    public DateTime DateTime { get; private set; }
    public decimal TemperatureCelcius { get; private set; }
    public decimal DewPoint { get; private set; }
    public string Condition { get; private set; }
    public int ConditionCode { get; private set; }
    public int WindSpeed { get; private set; }
    public string WindDirection { get; private set; }
    public decimal WindDegrees { get; private set; }
    public int UltravioletIndex { get; private set; }
    public decimal Humidity { get; private set; }
    public decimal WindChill { get; private set; }
    public int HeatIndex { get; private set; }
    public decimal FeelsLike { get; private set; }
    public decimal Snow { get; private set; }

    public HourlyForecastData(HourlyForecastDataState state)
    {
        DateTime = state.dateTime;
        TemperatureCelcius = state.temperatureCelcius;
        //...etc
    }
}

//Usage:
HourlyForecastDataState HFDstate = new HourlyForecastDataState();
HFDstate.temperatureCelcius = 100 //omg, it's hot!

HourlyForecastData HFD = new HourlyForecastData(HFDstate);

I actually thing this is a valid approach, why is it voted down? — Andre, Nov 08 '22 at 14:50

Lots of constructor parameters - Is there a better way?

3 Answers3