Too clever...?

That seems pretty standard to me, it isn't an optimization it's just the straightforward way to bounds-check a range, the readability does not really go down in my opinion. That said I would personally put the "lastIndexExclusive < firstIndexInclusive" check first since the validity of the other two checks does depend on that condition being true, and also should those indices really be signed ints?

Looks fine to me. In some other languages (like C# which I use most of the time) another common practice is to throw ArgumentException in those three cases.

That seems pretty standard to me, it isn't an optimization it's just the straightforward way to bounds-check a range, the readability does not really go down in my opinion. That said I would personally put the "lastIndexExclusive < firstIndexInclusive" check first since the validity of the other two checks does depend on that condition being true, and also should those indices really be signed ints?

Yes, you are right. I was thinking to myself that if I made them unsigned, I wouldn't even need the test for 'less than 0', since that would never happen in that case anyway.

EDIT: Sometimes I just mindlessly write code, but today I just had an epiphany.

Looks fine to me. In some other languages (like C# which I use most of the time) another common practice is to throw ArgumentException in those three cases.

Yes, I have had my dose of try..catch with Java back in the day. It is something I try to avoid, unless something really bad happens in the code, but I do see its use. Not sure why, but even 25 years back in time when I first learned Pascal, I tended to stick with the functions that didn't throw exceptions if for instance a file could not be found. Really never been a fan of exceptions. I think the main reason is that I think it clutters the code too much.

In java you even have to wrap string to integer conversions in a try...catch clause... I always was amazed by that even the "simplest" of operations has to be wrapped in exception handling in Java. You can probably guess that it is not my favourite programming language. If I were to rank, it would be c#, and Swift looks promising too, but both are (or have been) traditionally been tied to a single platform. So here I am with my second or third best: c/c++.

EDIT 2: Oh, yes, I get why you mentioned exceptions now. You are probably right that it is better to throw an exception rather than returning an arbitrary 'negative 1', or pass a value by reference to see if the operations succeeded or not. I still have a bad feeling about using too much exceptions though.

I know I am not alone in feeling this way. The whole Objective-C setup that Apple provides, is built on the assumption that excessive exception handling is evil. Having pass by reference error values is the rule rather than the exception there.

Then you have the reversed way where you return an ok/error code and pass the returned index as a reference.

What is best practice? Is there even such a thing in this regard? Swift can return tuples. That is a nifty thing. Too bad I am reluctant to be tied to a system that is so locked to one platform and one platform only.

This isn't too bad. Overly clever would be putting 0, first, last and length in an array, calling is_sorted() on it, and relying on the compiler to unroll the loop to get the same result. Or abusing operator overloading and expression templates to get 0 first last length to compile so that you can python style chained comparisons.

This isn't too bad. Overly clever would be putting 0, first, last and length in an array, calling is_sorted() on it, and relying on the compiler to unroll the loop to get the same result. Or abusing operator overloading and expression templates to get 0 first last length to compile so that you can python style chained comparisons.

Oh dear. :D Yes, that is overly clever. Not sure if it is any more efficient though. The first one seems to even have a bit more overhead than 5 if tests, the second one gives me a feeling to be at best just as good. I don't know the underlying behaviour of either is_sorted or the Python style chained comparisons, though.

std::is_sorted() works pretty much how you imagine. It loops through the sequence and checks if the first element is less than or equal to the second element and the second element is less than or equal to the third element and so on. In principle, if you have a fixed sized array, the compiler can unroll the loop and just do the checks on the individual elements. In practice, many compilers don't that kind of aggressive data flow analysis. Additionally, many standard library implementations don't implement is_sorted() in what you might consider the most straightforward manner, and instead implement in terms of comparing std::is_sorted_until() against last. This requires the addresses of the individual array elements, which compilers have a harder time optimizing out than the raw value comparisons.

In Python a < b < c has the exact same performance as a < b and b < c during execution, and is very slightly faster during compilation to byte code. With sufficiently aggressive inlining C++ expression templates would have the same run time as the raw operators, but would be massively slower during compilation. Ex: gcc 4.9.2 with -03 generates the same assembly code for comp1() and comp2() in the following code:

struct Lte {
  static bool process(int a, int  b) {
    return a <= b;
  }
};

template <typename T>
struct Operator {};

Operator<Lte> lte;

template <typename Op, typename Lhs>
struct Holder {
  Lhs lhs;
  int rhs;

  Holder(Lhs l, int r) : lhs(l), rhs(r) {}

  operator bool() {
    return Op::process(lhs, rhs);
  }
};

template <typename Op, typename Val>
struct Holder<Lte, Holder<Op, Val> > {
  Holder<Op, Val> lhs;
  int rhs;

  Holder(Holder<Op, Val> l, int r) : lhs(l), rhs(r) {}

  operator bool() {
    return lhs.operator bool() && (lhs.rhs <= rhs);
  }
};

template <typename Op, typename Val>
struct LeftHandProxy {
  Val lhs;

  LeftHandProxy(Val val) : lhs(val) {}
};

template <typename Op, typename Val>
LeftHandProxy<Op, Val> operator<(Val lhs, Operator<Op> rhs) {
  return LeftHandProxy<Op, Val>(lhs);
}

template <typename Op, typename Val>
Holder<Op, Val> operator>(LeftHandProxy<Op, Val> lhs, int rhs) {
  return Holder<Op, Val>(lhs.lhs, rhs);
}

bool comp1(int a, int b, int c, int d) {
  return a <= b && b <= c && c <= d;
}

bool comp2(int a, int b, int c, int d) {
  return a <lte> b <lte> c <lte> d;
}

This is slightly more complicated than necessary, but it compiles, produces the desired assembly code and I don't feel like spending more time on it just for a forum post. Adding other operators in addition to <= is left as an exercise for the reader, but should be fairly straightforward.

What is best practice? Is there even such a thing in this regard? Swift can return tuples. That is a nifty thing. Too bad I am reluctant to be tied to a system that is so locked to one platform and one platform only.

You should have an assert statement, unless you actually expect it to be valid to pass in bad bounds to that function during normal operation.

Swift can return tuples. That is a nifty thing. Too bad I am reluctant to be tied to a system that is so locked to one platform and one platform only.

C++ can return tuples too...

You can even use std::tie to unpack the returned tuple right into a set of variables.

Yes, you are right. I was thinking to myself that if I made them unsigned, I wouldn't even need the test for 'less than 0', since that would never happen in that case anyway.

Not only should they be logically unsigned, but they should also be size_t or (if you want to be properly pedantic) std::string::size_type.

The reason for this is because many string functions use std::string::npos as a constant. It's defined as (basically) size_t(-1).*

That is to say, the highest value a size_t variable can hold (since it's unsigned, and the -1 loops back around).

If you do this:

unsigned pos = myStr.find("blah");

if(pos == std::string::npos)
    return false;

...then ya got a bug. 'unsigned' and 'int' are both 32 bits even on 64 bit computers.* But 'size_t' is 64 bit on 64 bit computers.*

So if 'pos' is merely an 'unsigned' variable, it'd never be true.

if(my32BitInteger == TheHighestValueA64BitIntegerCanHold)
     return false;

*Usually, on modern personal computers, using the commonly-used compilers, but not guaranteed.

In my own code, if I'm making assumptions about variable sizes, I use uint32_t, uint64_t, and etc...

But when I'm using std::string's functions, I have to use std::string's assumptions (std::string::size_type - or in my code, I use size_t because it's most likely equivalent).

Yes, you are right. I was thinking to myself that if I made them unsigned, I wouldn't even need the test for 'less than 0', since that would never happen in that case anyway.

Not only should they be logically unsigned, but they should also be size_t or (if you want to be properly pedantic) std::string::size_type.

The reason for this is because many string functions use std::string::npos as a constant. It's defined as (basically) size_t(-1).*

That is to say, the highest value a size_t variable can hold (since it's unsigned, and the -1 loops back around).

If you do this:

unsigned pos = myStr.find("blah");

if(pos == std::string::npos)
    return false;

...then ya got a bug. 'unsigned' and 'int' are both 32 bits even on 64 bit computers.* But 'size_t' is 64 bit on 64 bit computers.*

So if 'pos' is merely an 'unsigned' variable, it'd never be true.

if(my32BitInteger == TheHighestValueA64BitIntegerCanHold)
     return false;

*Usually, on modern personal computers, using the commonly-used compilers, but not guaranteed.

In my own code, if I'm making assumptions about variable sizes, I use uint32_t, uint64_t, and etc...

But when I'm using std::string's functions, I have to use std::string's assumptions (std::string::size_type - or in my code, I use size_t because it's most likely equivalent).

If you can use C++11 you should really be using auto. Doing so guarantees the correct type (instead of "most likely equivalent") and also prevents you from accidentally using an uninitialized variable.

static inline int findToken(const int firstIndexInclusive, const int lastIndexExclusive,
                     const std::string string, const std::string oneOfChars) {

    unsigned long stringLength = string.length();
    
    if(firstIndexInclusive < 0){
        return -1;
    }
    
    if(lastIndexExclusive > stringLength){
        return -l;
    }
    
    if((lastIndexExclusive < firstIndexInclusive){
        return -1;
    }
    
    //searching for occurence of any of the 'oneOfChars' here
    
    return 0;
}

I don't program too much in C++, but I believe your code is decently readable. I would do something like what I did above as your conditional statement was the most difficult piece of code to read. Since you used a bunch of "OR" operations within the conditional I believe you could break that into separate statements for better readability.

I am not entirely sure why you return -1 and 0 but my assumption would be for true/false. If that is the case couldn't you return a Boolean value instead?

Also, you could look at other source code for inspiration... Program into your language not in your language. Just a thought.

http://docs.oracle.com/javase/7/docs/api/java/util/StringTokenizer.html