0

I have written a class to represent 'Treeview-like' data, which can be simplified as:

public class Item
{
    public string Name { get; set; }
    public KPI AssociatedKPI { get; set; }
    public List<Item> Children { get; set; }
}

To each 'branch' of the Tree, can be associated a KPI, which can be simplified as:

public class KPI
{
    public string Name { get; set; }
    public double Value { get; set; }
}

My database looks like

+------+-------+
+ Name + Value +
+------+-------+
+ foo  +   5   +
+ bar  +  4.56 +
+------+-------+

The tree structure, with the names of the KPIs is supposed to be filled in already.

I am struggling to decide where I should put the method to fill the values of the KPIs.

  • First solution: In the KPI class

SELECT value FROM KPI WHERE NAME = 'foo'

It seems like it respects the SRP, the KPI class is responsible for the values of the KPI.

However, each KPI is unaware of the existence of others, and I need to have as many queries as I have branches in my tree.

  • Second solution: In the Item class

I would get the names of all the KPIs in the descendants of this Item, and get a query looking like

SELECT Value FROM KPI WHERE Name IN ('foo', 'bar')

This way, I only need one per root of the tree (so usually, a single one).

My concern is that it breaks SRP, the Item should not be in charge of setting the values, should it?

In conclusion:

What would be the best way of doing it? Is there a third, better way I did not think about?

In this case, performance is not really an issue, the table and the tree are small, but I am learning, and would like to learn about the best practices.

Thanks,

  • By fill the values do you mean retrieving data from the db and creating an item with a KPI for each one? I would say you want a repository for that and the KPI or Item both should not know about your datasource – Lotok Nov 21 '17 at 17:12
  • How would you do it if you didn't have to think about SRP at all? – Robert Harvey Nov 21 '17 at 20:12
  • @James The structure and the KPI names are already available. The issue is to fill in the 'Value' property of all the KPIs in the Tree, from the DB. – Maxime Nov 22 '17 at 8:06
  • @RobertHarvey I would definitively use the second solution ie. use a method in the Item class to drill down the tree, and make a single query. – Maxime Nov 22 '17 at 8:06
  • Then I'd not worry too much about SRP. Finding the way that best meets your requirements is more important than trying to adhere to an arbitrary, ambiguous principle. The purpose of SRP is to get you to think about whether or not your class is doing too much, not to impose a fixed restriction. – Robert Harvey Nov 22 '17 at 15:59
1

I believe you're misunderstanding SRP, or at least are only applying it partially.


  • First solution: In the KPI class

It seems like it respects the SRP, the KPI class is responsible for the values of the KPI.

There are two responsibilities here: retrieving the value, and storing the value (in memory).

"It's related to the KPI" is not an accurate description of a reponsibility. Responsibilities are generally expressed as verbs: storing the data, logging the messages, printing the output, sorting the list, formatting the date, ...

  • Second solution: In the Item class

My concern is that it breaks SRP, the Item should not be in charge of setting the values, should it?

I have to agree with you here, Item is definitely not where you should put it. But your reasoning is somewhat flawed (in the same way as the previous bullet point)

What would be the best way of doing it? Is there a third, better way I did not think about?
In this case, performance is not really an issue, the table and the tree are small, but I am learning, and would like to learn about the best practices.

In the interest of learning, I'll keep the answer demonstrative but not implemented to an expert professional level.

You also didn't specify how you want to fill in the data, and I'd like my answer to focus on the intention of the code, rather than the implementation. So I'm going to mainly focus on structure and outset, rather than showing you specific code.


First of all

A minor philosophical reframing.

You've left a few gaps in your question, e.g. how the items are connected or what the main purpose of the system is. I'm filling in some blanks here based on experience and inference.

As I understand it, Item and KPI are two parts of the same entity. KPI is the actual meat and potatoes, the value of the object. Item, on the other hand, is merely a wrapper which helps you connect KPI values to each other (in a hierarchical structure).

Unless you've omitted an important part (or I'm glossing over it), I see no reason for Item and KPI to be two separate classes. For the rest of the answer (unless you can explain why this is not correct), I'm going to merge these into a single class:

public class KpiItem
{
    public string Name { get; set; }
    public double Value { get; set; }

    public List<KpiItem> Children { get; set; }

    // Personally, I prefer also having a link to the parent. 
    // But this is optional.
    public KpiItem Parent { get; set; }
}

Secondly, based on your table data, there is no explanation how the hierarchy of items is stored. Maybe this is stored in another table (e.g. Items) which you did not show us. Maybe you simply didn't get to developing that part yet.

Whatever the reason, we need to know how these are linked before we can retrieve them with the correct hierarchy. So I'm going to add a column to your table: the name of the parent.

+------+-------+-------------------+
+ Name + Value +  Parent_Name      +
+------+-------+-------------------+
+ foo  +   5   +  (null)           +
+ bar  +  4.56 +  foo              +
+------+-------+-------------------+

In other words: bar is a child of foo. foo is no one's child, it's a top-level element.


Third solution: A separate class made for handling data retrieval.

This is a much more SRP-friendly way to approach the problem.

Now, we haven't quite established what you want to retrieve:

  • A single element (by name)
  • A single element (by name) plus its direct children
  • A single element (by name) plus all of its descendants (children, grandchildren, greatgrandchildren, ...)

For demonstrative purposes, I'll show a way to retrieve a single element (by name) plus its direct children. If you want to get all descendants, it's a simple matter of implementing recursion (unless your dataset can have circular references).

Since you're using SQL queries, so will I. We will need two queries:

  • Retrieving an item based on its name.
  • Retrieving an item based on its parent's name.

The queries are relatively simple:

SELECT * FROM KPIITEMS WHERE NAME = 'foo'

SELECT * FROM KPIITEMS WHERE PARENT_NAME = 'foo'

Sidenote
Notice that I'm doing SELECT * instead of SELECT value. For your current example, you already know the name you're looking for, so you're only interested in the value.
But this quickly falls apart when you start looking for other things, e.g. the first alphabetical item, the item with the longest name, ... Because now, you don't know the name.

Generally, you'll want to create your KpiItem based on the returned query; not based on the filters you supplied. While one could logically explain the other; this only applies in rare cases and is not a good general approach. This is why I did SELECT *, so that the query returns all columns so we can fully populate the KpiItem.

Since the question doesn't focus on how to retrieve data from SQL, I'm going to omit it for simplicity's sake. Let's assume these methods exist:

public KpiItem GetItem(string query);
public List<KpiItem> GetItems(string query);

In other words, if we supply the correct query to this method, it gives us the needed objects.

An example implementation of this third class would be as follows:

public class DataFetcher
{
    public KpiItem GetKpiItemWithChildren(string name)
    {
        var theItem = GetKpiItem_ByName(name);

        theItem.Children = GetKpiItemsBy_ParentName(name);

        return theItem;
    }

    private KpiItem GetKpiItem_ByName(string name)
    {
        return GetItem("SELECT * FROM KPIITEMS WHERE NAME = '" + name + "'");
    }

    private List<KpiItem> GetKpiItems_ByParentName(string parentName)
    {
        return GetItems("SELECT * FROM KPIITEMS WHERE PARENT_NAME = '" + parentName + "'");
    }
}

And then you can use this class as follows:

var fetcher = new DataFetcher();

var fooItem = fetcher.GetKpiItemWithChildren("foo");

Sidenote
There are things missing for this example to be a properly usable (professional) piece of code. Most importantly, this does not protect against SQL injection.
You really should protect against SQL injection (read up on it when you're ready), but I've omitted it from the current example because (a) as a beginner you're likely not at that point yet and (b) I'm trying to keep the example as simple as possible so you can understand the intended structure.


Your example case is simple. Once you get into building applications that have many tables and considerably more complex data structures, this answer doesn't really apply anymore. But I'm inferring that you're currently looking for a beginner's explanation, not so much an expert-level implementation.

If you're interested in scaling upwards, a good place to start would be by reading up on Entity Framework and LINQ. But there's no need to rush, there is merit to focusing on the basics first (as they say, only the fools rush in).

| improve this answer | |
0

There's no mention where KPIs get their names from, and how

Either way, it's all about that data

  • if KPIs already know their own name, the parent Item also can and should retrieve the names list from the same provider. In this setup Item should be responsible for provisioning child KPIs with data, as long as we don't want to introduce ItemKPIProvider yet

  • if it's a local provider, or we simply don't want Item to have anything to do with its child KPIs' name data source, we can still pass a lambda to each KPI constructor to collect the names in an array. It's also a bit better as it won't require recursion if we need to scale it to multidimensional Item.Lists

Also,

  • Judging by the public accessors, the interface might be in violation with both the D and the I in SOLID
  • Considering SRP without the rest of the principles is not in itself very helpful. In fact, OOD will get very intertwined without a physical model or proven conventions
  • In general, form follows function, so imposing architectural restrictions too early will hinder the ability to experiment and prototype
| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.