CODEWITHSUNDEEP
c++math
claudiodfc
claudiodfc

Reputation: 146

Math: How to optimize this if so it looks cleaner and faster?

This solution looks ugly. I've tried to simplify using a vector and cycling a vector but would it be faster? I tried making up a formula to retrieve these numbers but I'm terrible at math!

if (SkillValue > 115 && SkillValue <= 135)
{
    return 100 / 2;
}
else if (SkillValue > 135 && SkillValue <= 160)
{
    return 100 / 3;
}
else if (SkillValue > 160 && SkillValue <= 190)
{
    return 100 / 4;
}
else if (SkillValue > 190 && SkillValue <= 215)
{
    return 100 / 5;
}
else if (SkillValue > 215 && SkillValue <= 295)
{
    return 100 / 6;
}
else if (SkillValue > 295 && SkillValue <= 315)
{
    return 100 / 9;
}
else if (SkillValue > 315 && SkillValue <= 355)
{
    return 100 / 10;
}
else if (SkillValue > 355 && SkillValue <= 425)
{
    return 100 / 11;
}
else if (SkillValue > 425 && SkillValue < 450)
{
    return 100 / 12;
}

return 100;
}

Upvotes: 0

Views: 88

Answers (3)

Chris Uzdavinis
Chris Uzdavinis

Reputation: 6131

It's hard to find an equation to compute your result because your ranges are not equal in size, and the divisor jumps at 295.

To make a more compact version, here's a way:

#include <algorithm>
#include <iterator>

// 296 repeats to compensate for the jump in your divisor
constexpr int ranges[] = {0, 115, 136, 161, 191, 216, 296, 296, 296, 316, 356, 426, 456};

int compute(int SkillValue) {
    if (SkillValue <= 115 or SkillValue >= 450) {
        return 100;
    }
    auto elt = std::upper_bound(std::begin(ranges), std::end(ranges), SkillValue);
    return  100 / (elt == std::end(ranges) ? 1 : elt - std::begin(ranges));
}

This computes the same output as your original function, but I can't actually say I like it any better.

Using a map, it's a little easier to read:

#include <map>

const std::map<int,int> ranges {
    {115, 1}, 
    {135, 2},
    {160, 3},
    {190, 4},
    {215, 5},
    {295, 6},
    {315, 9},
    {355, 10},
    {425, 11},
    {450, 12}
};

int compute(int SkillValue) {
    if (SkillValue <= 115 or SkillValue >= 450) return 100;
    auto elt = ranges.upper_bound(SkillValue-1);
    return  100 / (elt == std::end(ranges) ? 1 : elt->second);
}

Upvotes: 1

cigien
cigien

Reputation: 60228

There doesn't appear to be any pattern to the bounds for each if condition, or at least any pattern that would let you write this as a closed form check.

However, you are currently checking all the if conditions, which is O(n) where n is the number of conditions. You can do this in O(log(n)) by using std::lower_bound on a range to find the value of the denominator, like this:

std::array bounds { 115, 135, 160, 190, ... };
std::array dens {1, 2, 3, 4, ... , 1};  // extra 1 at the end to account for the 
                                        // case that none of the bounds match
auto it = std::lower_bound(std::begin(bounds), std::end(bounds), SkillValue);
return 100 / dens[std::distance(std::begin(bounds), it)];

Upvotes: 2

Albert Renshaw
Albert Renshaw

Reputation: 17902

Right off the bat, you are repeating logic in each conditional that can be refactored out of the previous conditionals by switching to the following form:

if (SkillValue <= 115) {
    return 100/1;
} else if (SkillValue <= 135) {// the 'else' explicitly means that it is already NOT <= 115, aka IS > 115, so no need to repeat with a '&&'
    return 100/2;
} else if (SkillValue <= 160) {
    return 100/3;
} else if (SkillValue <= 190) {
    return 100/4;
}//... and so on

Upvotes: 2

Related Questions