1
\$\begingroup\$

I need an example on writing this code with range based loops:

int pListPet[2] = {34001, 34051};   

int pListGlobal[] = {9001, 9002, 9003, 9004, 9005, 9006, 20011, 20091, 20092, 20093, 20094, 20095, 30000};

const char* strMapListGlobal[] = {"string1" , "string2"};

const char* strMapEventOx = "string3";                              

std::string stringName = CPythonBackground::Instance().GetWarpMapName();

for (int i = 0; i < _countof(strMapListGlobal); i++)
{
    #ifdef CHECK_STRING_3
    if (strMapEventOx == stringName)
    {
        if (0 <= rVictim.GetRace() && rVictim.GetRace() <= 7)
            return FALSE;
    }
    #endif

    if (strMapListGlobal[i] == stringName)
    {
        for (int i = 0; i < _countof(pListGlobal); i++)
        {
            if (rVictim.GetRace() == pListGlobal[i] || pListPet[0] <= rVictim.GetRace() && rVictim.GetRace() <= pListPet[1])
                return FALSE;
        }
    }   
}

Do you think this correctly uses std::string? Can this be optimized more or maybe written shorter?

Should I use this?

if (!strcmp(strMapEventOx, stringName))
\$\endgroup\$
2
\$\begingroup\$

I would not use range-based for loops for this task (if I could avoid it). Both your loops fit well with standard algorithms, and I think using them would make the code somewhat clearer.

I'd also use std::vector (or std::array) for the storage instead of using raw arrays.

std::vector<int> pListPet{34001, 34051};

std::vector<int> pListGlobal  {9001, 9002, 9003, 9004, 9005, 9006, 20011, 20091, 20092, 20093, 20094, 20095, 30000};

std::vector<std::string> strMapListGlobale {"string1" , "string2"};

Then I'd probably start with a little function named in_range, something like this:

template <class T, class cmp = std::less<T>>
bool in_range(T x, T lower, T upper) { 
    return !(cmp(x, lower) || cmp(upper, x));
}

Note that I've turned this into a template, so if we ever need to deal with some type other than int, we're ready (and the cost is pretty minimal). Also note that I've changed the comparisons so they use std::less<T> by default, but specified the comparison operator in a template parameter, so we could (for example) invert the comparison if we really wanted. Using only < is more or less traditional in generic C++ code, so many standard algorithms and containers only need that defined (and this is the same way).

As @Aluan already noted, the section of code in the #ifdef CHECK_STRING_3 block isn't affected by the loop, so it really belongs outside the loop. Using the previous function, it becomes:

if (eventOx == mapName && in_range(rVictim.GetRace(), 0, 7))
    return false;

That leaves us with this:

for (int i = 0; i < _countof(strMapListGlobal); i++)
{
    if (strMapListGlobal[i] == stringName)
    {
        for (int i = 0; i < _countof(pListGlobal); i++)
        {
            if (rVictim.GetRace() == pListGlobal[i] || pListPet[0] <= rVictim.GetRace() && rVictim.GetRace() <= pListPet[1])
                return FALSE;
        }
    }   
}

Looking at this, the first thing that's noticeable is that the loop only does something when strMapListGlobal[i] == stringName. What's done in that case doesn't depend on the value that's found, just (apparently) the fact that it was found. That being the case, we can express the intent more directly something like this:

auto const &s = strMapListGlobal;

if (std::find(s.start(), s.end(), stringName) != s.end())
    // do something

Then we need to look at what happens inside the loop:

for (int i = 0; i < _countof(pListGlobal); i++)
{
    if (rVictim.GetRace() == pListGlobal[i] || pListPet[0] <= rVictim.GetRace() && rVictim.GetRace() <= pListPet[1])
        return FALSE;
}

So this is looking for a value in pListGlobal that equals rVictim.GetRace(), and if it finds that, checking whether rVictim.GetRace() is within a range defined by pListPet. Again, we can express that quite a bit more directly:

auto const &g = pListGlobal;
auto const &r = rVictim.GetRace();
auto const &p = pListPet;

if (std::find(g.begin(), g.end(), r) != g.end() && in_range(r, p[0], p[1]))
    return false;

I'm going to consider one further refinement from there. At least as you've shown them, pListGlobal and strMapListGlobale are both sorted. I'm going to assume we can depend on that being true--and that while you've shown only small lists here, that in real use they can actually be larger (perhaps much larger). If those are true, we can probably benefit quite a bit from using a binary search instead of a linear search. Using that, we can collapse this section of code down to:

if (std::binary_search(s.start(), s.end(), stringName) &&
    std::binary_search(g.begin(), g.end(), r)          &&
    in_range(r, p[0], p[1]))
{
    return false;
}

That's fairly concise, and if we deal with large lists of number, could be considerably faster as well, since we've replaced an \$O(N)\$ algorithm with one that's \$O(log N)\$ instead.

Naming

I really don't like having two nested for loops, both using i as their index. The compiler won't object, but it's quite confusing to read.

Other names seem...ugly at best. Not sure what the p in pListPet and pListGlobal are supposed to mean. Names should reflect the meaning of the contents, and it seems quite unlikely that these do. Likewise, strMapListGlobale looks like a name generated by picking three words at random, then tacking a useless str wart onto the beginning. I've no idea what it's really supposed to mean or contain.

Just as general guidance, names should refer to what this is at a more logical level, not just to the stored bits. That is to say, they usually shouldn't refer to how you're storing something (e.g., the data structure being used) but to the actual meaning of the data.

For example, when I look at the definition of pListGlobal, I can see that it's outside any function, so it's global. I can see it's an array because it has a pair of square brackets and a bunch of initializers. Repeating that information in the name doesn't really add anything new or useful. The name should normally tell me what 9001 vs. 9002 really means. Are these references to ISO 9000 series standards? Are they amounts of crude oil processed in a particular refinery at particular points in time? Maybe it's a number of toy wagons a factory produced each month. Based on the current names, pListPet and pListGlobal really could be any of the above, or just about anything else. The name doesn't give me a clue, and it definitely should (not just give me a clue, but by strong preference should tell me outright).

\$\endgroup\$
2
  • \$\begingroup\$ You should probably consider adding some advice on naming, as simply saying that the names are 'ugly' does not really help \$\endgroup\$
    – Kodnot
    Sep 23 '16 at 18:12
  • \$\begingroup\$ good answe. You are a pro. \$\endgroup\$
    – Takashi
    Oct 10 '16 at 14:01
6
\$\begingroup\$

Range for can be used with iterators, including pointers, as follows

for (auto name: strMapListGlobal)
{
    #ifdef CHECK_STRING_3
    if (strMapEventOx == stringName)
    {
        if (0 <= rVictim.GetRace() && rVictim.GetRace() <= 7)
            return FALSE;
    }
    #endif

    if (name == stringName)
    {
        for (auto p: pListGlobal)
        {
            if (rVictim.GetRace() == p ||
                pListPet[0] <= rVictim.GetRace() && 
                rVictim.GetRace() <= pListPet[1])
                return FALSE;
        }
    }   
}

Your program needs further refinement. Upon inspection, we see that the first conditional inside the loop body

if (strMapEventOx == stringName)
{
    if (0 <= rVictim.GetRace() && rVictim.GetRace() <= 7)
            return FALSE;
}

is invariant inside the loop and can be factored out. That leaves us with the following

for (auto name: strMapListGlobal)
{
    if (name == stringName)
    {
        for (auto p: pListGlobal)
        {
            if (rVictim.GetRace() == p ||
                pListPet[0] <= rVictim.GetRace() && 
                rVictim.GetRace() <= pListPet[1])
                return FALSE;
        }
    }   
}

To further refine your program, eliminate raw pointers. Not knowing what your macros do it is difficult to say but you might consider.

auto pets = std::vector<int> { 34001, 34051 };   

auto globalPets = {
    9001, 9002, 9003, 9004, 
    9005, 9006, 20011, 
    20091, 20092, 20093, 
    20094, 20095, 30000
};

auto globalEventNames = { "string1" , "string2" };

auto eventOx = "string3";                              

auto mapName = CPythonBackground::Instance().GetWarpMapName();

#ifdef CHECK_STRING_3
if (eventOx == mapName)
{
    if (0 <= rVictim.GetRace() && rVictim.GetRace() <= 7)
        return FALSE;
}
#endif


for (auto event: globalEventNames)
{
    if (event == mapName)
    {
        for (auto pet: globalPets)
        {
            if (rVictim.GetRace() == pet || pets[0] <= rVictim.GetRace() && rVictim.GetRace() <= pets[1])
                return FALSE;
        }
    }   
}

as for strcmp

if (strcmp(strMapEventOx, stringName))

avoid it!

if (strMapEventOx == stringName)

is more readable, more idiomatic, and likely faster.

\$\endgroup\$

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.