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

Calculating the number of elements of an underlying data structure

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

Problem

I read many comments on this question that one really shouldn't use local classes for functionality like which my code provides, but I couldn't really put things into perspective and decide what better alternatives are there, if any (in terms of solving the problem locally). Some mentioned lambdas, although they also pinpointed that with recursion problems can arise if it is declared with the auto keyword. I would like to keep the logic inside of this method. Is it clean code or not? What do you think? (please don't mention (int)size_t casting; it's personal)

The algorithm itself calculates the number of elements of the underlying data structure, excluding folders (AnyMap instances).

typedef std::map AnyMap; 
using boost::any_cast;

class MyExtendedAnyMap
{
private:
    AnyMap data; // stores values and other AnyMaps as well that act as a folder basically

public:
    int size() const
    {
        if(data.empty())
            return 0;

        int calcedSize = 0;

        struct GoRec{
            static void go(const AnyMap& anyMap, int& calcedSize){
                for(const auto& kv : anyMap){
                    if(kv.second.type() == typeid(AnyMap)){
                        go(any_cast(kv.second), calcedSize);
                    }
                    else{
                        calcedSize++;
                    }
                }
            }
        };

        for(const auto& kv : data){
            if(kv.second.type() == typeid(AnyMap)){
                GoRec::go(any_cast(kv.second), calcedSize);
            }
            else{
                calcedSize++;
            }
        }

        return calcedSize;
    }

    // ...
}

Solution

Don't Repeat Yourself

Your looping code appears twice. Once in the body of size() and once in the body of GoRec::size(). You only need it once. In fact, the entire structure of GoRec is kind of weird and unnecessary. I also don't like how you named the function go(), made it void, and have it take a reference... Instead, let's write a free function:

size_t size_of(AnyMap const& data)
{
    size_t size = 0;
    for(const auto& kv : data){
        if(kv.second.type() == typeid(AnyMap)) {
            size += size_of(any_cast(kv.second));
        }
        else{
            size++;
        }
    }        
    return size;
}


That way:

int size() const {
    return size_of(data);
}


And we have no repetition and no unnecessary extra case for empty map.

Know Your Interface

There are several different overloads for boost::any_cast. You are using the wrong one. This is the one you want:

template const ValueType * any_cast(const any * operand);


This overload will not throw, and will return a null pointer if you're attempting to cast to the wrong type. Now, the overload you're using will throw if it's the wrong type - but you're explicitly checking that first. Effectively, you're re-implementing what any_cast is doing under the hood, which is bad.

If you used the other overload, you could let the any internal implementation remain private. And the code ends up looking more direct:

size_t size_of(AnyMap const& data)
{
    size_t size = 0;
    for(const auto& kv : data){
        if (auto submap = any_cast(&kv.second)) {
            size += size_of(*submap);
        }
        else{
            size++;
        }
    }        
    return size;
}

Code Snippets

size_t size_of(AnyMap const& data)
{
    size_t size = 0;
    for(const auto& kv : data){
        if(kv.second.type() == typeid(AnyMap)) {
            size += size_of(any_cast<const AnyMap&>(kv.second));
        }
        else{
            size++;
        }
    }        
    return size;
}
int size() const {
    return size_of(data);
}
template<typename ValueType> const ValueType * any_cast(const any * operand);
size_t size_of(AnyMap const& data)
{
    size_t size = 0;
    for(const auto& kv : data){
        if (auto submap = any_cast<AnyMap>(&kv.second)) {
            size += size_of(*submap);
        }
        else{
            size++;
        }
    }        
    return size;
}

Context

StackExchange Code Review Q#107434, answer score: 2

Revisions (0)

No revisions yet.