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 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<MainStep> MainSteps { get; set; }
    public List<SequenceStep> SequenceSteps { get; set; }
    public List<SubSequenceStep> SubSequenceSteps { get; set; }
    public List<ConfigurationRow> ConfigurationRows { get; set; }

    public List<Tag> 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<int, StepArguments> 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].Arguments[120].HighLimit = 200;

While this would make clearer code it would also ruin the purpose of having the Configurations class in the first place.

share|improve this question
    
How are subsequences related to sequences? Does each sequence have its own set of subsequences? –  Carlos Jul 25 at 8:36
    
@Carlos Yeah. The main steps actually consist of sequence steps which then consists of subsequence steps. We thought about mashing em all together but decided not to since we mainly look at sequence steps when doing calculations/iterations. I hope that makes sense. –  Hjalmar Z Jul 25 at 8:44
1  
Not really knowing your domain, that looks like a wrong decision. How do you even know which SubSequenceStep is related to which SequenceStep? By the way, maybe you could give a general overview of what all those classes are doing, including the ConfigurationSheet. –  toto2 Jul 25 at 11:50
    
I also think it's a terrible decision. –  Shane Hsu Jul 25 at 20:04
    
I hear what you're saying and like I've said before I initially wanted to do it the way you're all describing. Yes, in theory they all connect but in the actual database they don't. It's hard to explain when you don't see the full picture but just trust me on this, please. –  Hjalmar Z Jul 29 at 7:10

2 Answers 2

up vote 7 down vote accepted

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<SequenceStep> 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<SequenceStep> 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.

  1. If you have control over your data, you can try to change its format to simplier one.
  2. 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.
  3. You can use shorter naming. Do you really need to have ConfigurationSheet.ConfigurationRow, for example? Isn't ConfigurationSheet.Row just as readable?
  4. Also if Arguments is an equivalent of individual "cells", then using indexers makes sense: sheet[1][120]
share|improve this answer
    
Thanks for the advise, I've been thinking along the same lines. It doesn't really address my problem though. If you take a look at my final code example, would you recommend shortcuts in the StepArguments-object to allow something like sheet.ConfigurationRows[1].Arguments[120].HighLimit = 200; instead? –  Hjalmar Z Jul 25 at 10:15
    
@HjalmarZ, i've made an edit. –  Nikita Brizhak Jul 25 at 11:19

Here's my thoughts on it. Unfortunately it's a bit vague, but I'll continue to think about it.

  • Maintainability: The goal posts on this kind of thing can often change. Right now, you have a couple of levels under the root, which can be managed pretty easily with what you've written. You have to judge whether the boss is going to come back to you and ask for something a little more complex. This is actually the bit I find the hardest about programming. You don't want to waste time, but you can waste time by building things out as well as by not building them out.

  • Complexity: Suppose it does get more complex, and you're doing Step[i].substep[j].substep[k].substep[l]... etc. At some point this will get confusing and hard to debug. Have a think about some sort of hierarchical structure which effectively turns the code into a 2-step process: check for children, or fill in the information. This would probably look like a database table of some sort (or LINQ, if you like that) with some columns specifying where a given node is, ie a table representing a tree (might be an idea to find some pre-existing tree code).

  • Models and meanings: Have a think about why you have multiple layers. If each layer is different to the next, and has clear purpose, then actually what you've written is ok. Because it's easier to debug something when you know what it's supposed to do. This is another piece of vague advice, admittedly.

share|improve this answer

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.