patterncppMinor
Calculating the number of elements of an underlying data structure
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
The algorithm itself calculates the number of elements of the underlying data structure, excluding folders (
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
That way:
And we have no repetition and no unnecessary extra case for empty map.
Know Your Interface
There are several different overloads for
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
If you used the other overload, you could let the
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.