patterncsharpMinor
Too many objects, too "deep" class structure?
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:
These types are all mine and comes with the library, for example the
And it continues like this. Here's the
The
This could, potentially, result in code like this:
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:
Which would allow this:
```
if(sheet.SequenceStep[0].SetPoint > 0)
sheet.ConfigurationRows[1].
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
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
(or you can use Linq).
I think the line
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.HighLimitinstead. 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'tConfigurationSheet.Rowjust as readable?
- Also if
Argumentsis 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.