HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Too many objects, too "deep" class structure?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
objectstoodeepstructuremanyclass

Problem

I have a pretty large model library I'm working on and it's turning out quite complex, or so I believe at least and I could use a second opinion.

The basic idea is that the user should only have to create an instance of a big base class and then build on that with the other object types.

These are some of the properties in the base class:

public class ConfigurationSheet
{
    public List MainSteps { get; set; }
    public List SequenceSteps { get; set; }
    public List SubSequenceSteps { get; set; }
    public List ConfigurationRows { get; set; }

    public List PLCTags
    {
        get { return ConfigurationRows.Select(x => x.PLCTag).ToList(); }
    }
}


These types are all mine and comes with the library, for example the ConfigurationRow-object looks like this:

public class ConfigurationRow
{
    public Tag PLCTag { get; set; }
    public Dictionary Arguments { get; set; }
}


And it continues like this. Here's the StepArguments object:

public class StepArguments
{
    public Configurations Configurations { get; set; }
    public Calculations Calculations { get; set; }
}


The Configurations object actually goes another two steps deeper.

This could, potentially, result in code like this:

if(sheet.SequenceStep[0].DefaultConfigurations.Tolerances.SetPoint > 0)
    sheet.ConfigurationRows[1].Arguments[120].Configurations.Tolerances.HighLimit = 200;


And this doesn't seem right to me. Should I perhaps break some of my objects and put the properties in bigger classes, or should I create shortcut properties/methods/indexers or am I maybe just overreacting?

I'm thinking maybe something like this:

public class StepArguments
{
    public Configurations Configurations { get; set; }
    public Calculations Calculations { get; set; }
    public double HighLimit { get { return Configurations.Tolerances.HighLimit; } }
}


Which would allow this:

```
if(sheet.SequenceStep[0].SetPoint > 0)
sheet.ConfigurationRows[1].

Solution

I think you should definitely keep class structure as close as possible to actual data structure. So, for example, if your MainStep "contains" SequenceStep, you should reflect that in your code by using aggregation:

class MainStep
{
    public List SequenceSteps { get; set; }
    .....
}


etc...

If you want an easy access to some parts of your data, you should implement additional methods on highest level. For example, to iterate through SequenceSteps, you can implement following extension method:

public static IEnumerable GetSequenceSteps(this ConfigurationSheet sheet)
{
    foreach(var main in sheet.MainSteps)
    {
        foreach(var sub in main.SequenceSteps)
        {
            yield return sub;
        }
    }
}


(or you can use Linq).

I think the line sheet.ConfigurationRows[1].Arguments[120].Configurations.Tolerances.HighLimit is fine, as long as it reflects the actual data structure. Its definetely not the most complex thing in the world, far from it (mappings for complex databases often have way more layers of aggregation :) ). There is not much that can be done.

  • If you have control over your data, you can try to change its format to simplier one.



  • You can also try to move the logic to lower level, where possible. So it will look like row.Arguments[120].Configurations.Tolerances.HighLimit instead. Its hard to tell an exact refactoring without seeing the bigger picture. Using "shortcuts" for commonly accessed fields is fine as well, as long as you don't overdo it. Use extension methods for that where possible, so that class structure remains clear.



  • You can use shorter naming. Do you really need to have ConfigurationSheet.ConfigurationRow, for example? Isn't ConfigurationSheet.Row just as readable?



  • Also if Arguments is an equivalent of individual "cells", then using indexers makes sense: sheet[1][120]

Code Snippets

class MainStep
{
    public List<SequenceStep> SequenceSteps { get; set; }
    .....
}
public static IEnumerable<SequenceStep> GetSequenceSteps(this ConfigurationSheet sheet)
{
    foreach(var main in sheet.MainSteps)
    {
        foreach(var sub in main.SequenceSteps)
        {
            yield return sub;
        }
    }
}

Context

StackExchange Code Review Q#58016, answer score: 7

Revisions (0)

No revisions yet.