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

Find a Value in a Container

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

Problem

I have created a simple function to determine if a container contains a value. It allows for certain conditions to be set as well: where to to start searching for value, which way to search, whether to find the first or the last occurrence of the value, and whether or not to include the starting point. I want its return value to be either the index of the matching element, or -1 if no matches were found.

template
int Vec_Find(const Container& c, int size, const T& val, int pivot_idx = 0, 
             bool find_first = true, bool ltr = true, bool include_pivot = true)
{
    if (find_first && ltr)
    {
        if (include_pivot) pivot_idx++;
        int i = std::distance(std::begin(c), 
                    std::find(std::begin(c) + pivot_idx, std::end(c), val));
        if (i != size) return i;
    }
    else if (find_first && !ltr)
    {
        if (!include_pivot) pivot_idx--;
        int i = std::distance(std::find(std::rbegin(c) + (size - pivot_idx - 1), 
                                        std::rend(c), val), std::rend(c) - 1);
        if (i != -1) return i;
    }
    else if (!find_first && ltr)
    {
        if (include_pivot && (c[pivot_idx] == val)) return pivot_idx;
        int i = std::distance(std::find(std::rbegin(c), 
                            std::rend(c) - (pivot_idx + 1), val), std::rend(c) - 1);
        if (i != pivot_idx) return i;
    }
    else
    {
        if (include_pivot) pivot_idx++;
        int i = std::distance(std::begin(c), 
                    std::find(std::begin(c), std::begin(c) + pivot_idx, val));
        if (i != pivot_idx) return i;
    }
    return -1;
}


I am a student to C++ and am looking for ways to code better, I appreciate all suggestions.

To Editors: I wasn't sure if I should leave my long lines on one line or fit to screen, please let me know if they should be on one line, and I will update it.

Solution

Parameters

As a starting point, I'd usually advise against passing bools as parameters--especially passing more than one bool to a particular function. I have better ways to spend my time than memorizing the meaning of:

foo(..., false, false, true);


vs.

foo(..., false, true, false);


I suppose I could barely see something like this if you were doing something like passing some bools, and it was putting them together into a bit set, so true, false, true basically just meant 101, but for something like this where each has a unique meaning, it seems unusually difficult to read.

Basic Utility

At least in my opinion, however, that nearly pales compared to the mere fact that I don't see much point in the function existing at all.

People who are accustomed to C++ are generally fairly accustomed to how the standard library works. Most of use would generally rather get an iterator than an index. If we want to search from end to beginning, passing rbegin() and rend() to find makes that intent fairly clear.

For example, if I want to search for the last item, starting 4 from the end, I'd have a much easier time understanding this:

auto p = std::find(foo.rbegin()+4, foo.rend(), value);


...rather than:

int p = Vec_find(foo, foo.size(), value, 4, false, true, true);


or:

int p = Vec_find(foo, foo.size(), value, 4, true, false, true);


[At least as I read things, those are basically the same--either find the last item, or find the first, starting from the end. Oh, and pardon me if I'd passed the wrong thing for the size parameter--I couldn't figure out exactly what it was supposed to do.]

Granted, iterators do tend to lead to fairly verbose code, but in this case, using your code can be even more verbose. I'm pretty sure when ranges become generally available (and sooner, for people who only need to target one compiler, or don't mind using Boost) quite a few people will welcome them with open arms. They can reduce verbosity quite a bit. Given a choice, however, between using this and using the standard library directly, I'd take the standard library without a second thought.

Code Snippets

foo(..., false, false, true);
foo(..., false, true, false);
auto p = std::find(foo.rbegin()+4, foo.rend(), value);
int p = Vec_find(foo, foo.size(), value, 4, false, true, true);
int p = Vec_find(foo, foo.size(), value, 4, true, false, true);

Context

StackExchange Code Review Q#157208, answer score: 8

Revisions (0)

No revisions yet.