1
\$\begingroup\$
#include <iostream>
#include <vector>
#include <algorithm>


template <typename T>
int copy_max(T& to, T& from, unsigned char delim, size_t limit)
{
    typename T::iterator pos = std::find(from.begin(), from.end(), delim);
    typename T::iterator prev_pos = pos;

    if(pos == from.end())
    {
        return -1;
    }

    size_t index = std::distance(from.begin(), pos);

    while(pos != from.end() 
    && index <= limit)
    {
        prev_pos = pos;
        pos = std::find(pos + 1, from.end(), delim);
        index = std::distance(from.begin(), pos);
    }

    index = std::distance(from.begin(), prev_pos);

    if(index == 0)
    {
        return -1;
    }
    to.insert(to.begin(), from.begin(), prev_pos);
    from.erase(from.begin(), prev_pos);
    return index;
}

void output(const std::vector<unsigned char>& va, const std::vector<unsigned char>& vb )
{
    std::string str(va.begin(), va.end());
    std::cout << "To: " << str << std::endl;
    str.assign(vb.begin(), vb.end());
    std::cout << "From: " << str << std::endl;  
}
static int case_num = 1;
#define expect(a, b) \
std::cout << "[" << case_num ++ << "] "; \
if(a == b) \
std::cout << #a << " == " << #b << " TRUE " << std::endl;\
else {\
std::cout << "FAIL " << #a << " != " << #b << " FALSE " << std::endl;\
throw new int;\
}

int main() {
    std::string b = "Hi\x24The\x24HowAreYou\x24?";
    std::vector<unsigned char> va;
    std::vector<unsigned char> vb(b.begin(), b.end());

    int index = copy_max(va, vb, '\x24', 5);
    expect(index, 2);
    output(va, vb);

    va.clear();
    index = copy_max(va, vb, '\x24', 5);
    expect(index, 4);

    va.clear();
    index = copy_max(va, vb, '\x24', 5);
    expect(index, -1);

    return 0;
}

Don't mind the main() function and testing macros, this is just something I threw together in a matter of minutes.

\$\endgroup\$
1
  • \$\begingroup\$ One comment about the testing macro, if I was writing it it would probably return false for failure and test as much as possible. I would also expect a catch somewhere for the code you do have. \$\endgroup\$
    – pacmaninbw
    Sep 13, 2016 at 17:29

2 Answers 2

6
\$\begingroup\$

The function copy_max() is mis-named, No one maintaining this code would know from the name of the function that the from string is being shortened by the from.erase(from.begin(), prev_pos). Copy does not imply destruction of the source, move might be more appropriate.

You may also want to consider the Single Responsibility Principle and break copy_max() into multiple functions. I see at least 2 functions there. This loop is complex enough to be it's own function and can be called from copy_max()

    while(pos != from.end() && index <= limit)
    {
        prev_pos = pos;
        pos = std::find(pos + 1, from.end(), delim);
        index = std::distance(from.begin(), pos);
    }

Doing so makes the logic of copy_max() clearer.

I'm fairly sure that you can calculate index only once. I'm not sure why it's being calculated in 3 different places in copy_max().

The title is miss leading as well, since this will copy more than just characters.

\$\endgroup\$
2
  • \$\begingroup\$ Can you elaborate on "at least 2 functions"? You mean moving and calculating index or something else? \$\endgroup\$ Sep 13, 2016 at 19:10
  • \$\begingroup\$ I've expanded the answer to answer you comment. \$\endgroup\$
    – pacmaninbw
    Sep 13, 2016 at 19:19
2
\$\begingroup\$
#include <iostream>
#include <vector>
#include <algorithm>

Keep your headers organized for readability and maintainability.

Any source files you generate should be self-sufficient by including the headers of any symbol used. While some files will include others (e.g. <iostream> including <string>), the behavior is not guaranteed. You are missing:

#include <cstddef>     // std::size_t
#include <iterator>    // std::distance
#include <string>      // std::string

template <typename T>
int copy_max(T& to, T& from, unsigned char delim, size_t limit)

Defining the interface is the most important step when designing a function. Doing things that are different or surprising will confuse and annoy users of your functions.

The name of the function doesn't match what going on in the body of the function. Copy is a non-destructive action on the source. When you copy one item, you have two of that item. What you are actually doing is copying a subrange then destroying the subrange source (effectively moving the subrange).

Whenever you are writing functions, prefer to write parameters in the order that replicates similar functions from that language's standard library. Most users of your code are more than often going to use standard library code than handwritten code. Having to deal with alternating order of parameters is annoying if your function wants destination before source in the arguments while the majority of code is using source before destination.

What does T really represent? Semantically it refers to any type T. As a user, I would have to look into your implementation and dig through the documentation to figure out what exactly T represents. Be explicit. Expecting a container type? Call it Container (or C if your coding standard allows localized shortened variables).

Specify immutable variables with const (or constexpr for literal types).

Must the types for to and from really be the same type? Perhaps it would be better to loosen that restriction to the underlying types being convertible.

Your return type narrowly converts from std::size_t to int.


while(pos != from.end() 
&& index <= limit)

Use space to distinguish language constructs from function calls.

while (post != from.end()
       && index <= limit)

typename T::iterator pos = std::find(from.begin(), from.end(), delim);

Prefer auto to avoid redundant repetition of type names.

In generic programming, prefer non-member functions to member functions. Member functions begin and end are fine when you have container types that adhere to the standard. Some containers just don't provide those facilities (like C-Arrays). C++ provides non-member std::begin() and std::end() to help with language container types. To support user container types with non-member begin/end, ADL can be used.

using std::begin;
using std::end;
auto pos = std::find(begin(from), end(from), delim);

Avoid std::endl. Directly state your intent in code and be aware of what std::endl does. The following two statements are equivalent (ignoring localization).

std::cout << std::endl;
std::cout << '\n' << std::flush;

Again, be explicit with your intent.

std::cout << std::endl;           // Flush intended?
std::cout << '\n';                // Definitely don't flush.
std::cout << '\n' << std::flush;  // Definitely flush.

Don't use macros for functions. Macros don't obey argument passing, scope, or type rules. Macros may be interpreted differently by the compiler compared to the reader's interpretation.

Prefer throwing a standard-library exception type or a type derived from the std::exception hierarchy.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.