3

Assume I have some data a csv-Files like

ObjectName, PropertyA, PropertyB, PropertyC
"Name1", 3, 1, 4
"Name2", 1, 5, 9
"Name3", 2, 6, 5
...

and a typical question I want to answer would be

For which Object is PropertyX maximal?

I have two approaches for this and would be thankful for some Input.


Approach 1

I define a class like

struct Object {
    std::String name;
    int a;
    int b;
    int c;
};

Store the data in a class like

class ObjectCollection {
    std::vector<Object> collection;
}

And provide two functions

size_t ObjectCollection::getMaxIndexOfA()
size_t ObjectCollection::getMaxIndexOfB()
size_t ObjectCollection::getMaxIndexOfC()

Now these Functions would essentially be the same and look like

size_t maxIndex = -1;
int max = std::numeric_limits<int>::min();
for (size_t i = 0; i < collection.size(); ++i) {
    if (collection[i].a > max) {
        maxIndex = i;
        max = collection[i].a; 
    }
}
return maxIndex;

It bothers me that I would have to write and maintain the same code twice.


Approach 2

Store the data in a class like

class ObjectCollection {
    std::vector<String> names;
    std::vector<int> a;
    std::vector<int> b;
    std::vector<int> c;
}

Then I could provide methods like

const std::vector<int>& ObjectCollection::getA() const;
const std::vector<int>& ObjectCollection::getB() const;
const std::vector<int>& ObjectCollection::getC() const;

And use a single function to find the maximum which I have to call as

getMaxIndex( collection.getA() );

where size_t getMaxIndex(const std::vector<int>&) would be essentially the same as in approach 1.


I think I prefer the second approach, but it bothers me that in this case there is no class representing a single Object.

Is it weird/bad design to store the data as in the second approach? Is there another smart approach I didn't think about?

By the way, I am more concerned with the choice between these two approaches than the fact that I should probably use std::max_element to find the index.

1
  • 6
    Your only criticism of approach 1 is “It bothers me that I would have to write and maintain the same code twice”, yet you exclude template functions such as std::max_element() which address exactly this problem? Especially the overload that takes a custom comparator. Don't contort your design when easy solutions are available in the standard library.
    – amon
    Commented Dec 13, 2017 at 10:03

4 Answers 4

3

Is there another smart approach I didn't think about?

If your properties are of the same type and behave similar, why not address them by an index?

enum PropIndex{PropertyA=0, PropertyB=1, PropertyC=2};

struct Object {
    std::String name;
    std::vector<int> property(3); // use PropIndex to access the value you want

    // if that helps for convenience, you can also add getters and setters like this one 
    int getA(){ return property(PropertyA); }
    //  ... but then you should consider to make the public members 
    //  ... all private, and provide proper public getters/setter for all of them
};

Now you need to implement your maximum function only once (with an additional parameter which Property you want).

1
  • +1.. was wondering, though, if there's exactly three wouldn't an array be just as effective, and simpler?
    – Erik Eidt
    Commented Dec 13, 2017 at 17:23
1

Approach 1 is unarguably the better design - it represents the "reality" of the data much better, which will make it much easier for future developers to understand.

1

So have both. There's no written rule that you must stick with the model you've chosen. You keep ObjectCollection class and have a method like getObject(int index) return a Object instance for all values of that row.

Your getObject method would simply have to lookup the values laterally across all vectors and put their references in Object class.

If you think you might need to call this method a lot, consider representing the data in both ways. Yes, that is to say, make ObjectCollection class hold vector column data as well as a single vector with Object instances. Obviously you'd be using up more memory, but this is the old memory-performance tradeoff. If you feel performance is more important here, don't hestitate to do it this way!

1

Domain goes first !

The design should aim at correctly representing the domain, before going into tactical considerations.

So approach 1 is the way to go. It perfectly reflects the real structure of your data, and it enforces separation of concerns between Collection and Object. For example, you could easily change the collection's container (e.g. replacing array with a map or a linked list) or the objects's properties (e.g. collection[i].average_abc()).

DRY using pointers to member

You can write a private helper function that would work with a pointer to member instead of a being bound to a specific member:

class ObjectCollection {
    size_t getMaxIndexOf(int Object::* o); // private helper function 
public:
    std::vector<Object> collection;
    size_t getMaxIndexOfA();
    size_t getMaxIndexOfB();
    size_t getMaxIndexOfC();
};

A pointer to member is a very special beast: it is relative to an object, so that you can use the same pointer and combine it with all items of your collection:

size_t ObjectCollection::getMaxIndexOf(int Object::* o) {
    size_t maxIndex = -1;
    int max = std::numeric_limits<int>::min();
    for (size_t i = 0; i < collection.size(); ++i) {
       if (collection[i].*o > max) {  // <---- use of the pointer to member
           maxIndex = i;
           max = collection[i].*o;  // <---- use of the pointer to member
       }
    }
    return maxIndex;
}

You can then reuse your helper function as follows:

size_t ObjectCollection::getMaxIndexOfA() {
    return getMaxIndexOf(&Object::a);
}
size_t ObjectCollection::getMaxIndexOfB() {
    return getMaxIndexOf(&Object::b);
}
size_t ObjectCollection::getMaxIndexOfC() {
    return getMaxIndexOf(&Object::c);
}

Here an online demo

DRY using standard algorithms

As suggested by Amon in the comments, you could just use std::max_element():

size_t ObjectCollection::getMaxIndexOfA() {
    auto it = std::max_element(collection.begin(), collection.end(), 
                            [](auto &o1, auto &o2){ return o1.a<o2.a; });
    return std::distance(collection.begin(), it);
}

This standard algorithm returns an iterator, but as shown, you can convert it back to an index.

Online demo

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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