Things to consider (std::string)

[Suzie999]

Hi, I am currently writing a library for dynamic use, and in it I have some string functions.

This particular one returns a substring of a given string from the rightmost, determined by a given count.

I just get the feeling that I'm missing some error checking and would like to hear any comments or tips if you have the time.

unsigned int __stdcall sRightmost(char * source, unsigned int len, unsigned int count, char * buffer){

    //convert to string
    std::string String(source);

    //check input is not empty
    if (String == "") {
        return 0;
    }

    //get length once only
    unsigned int slen = String.length();

    //check count is < len
    if (count > slen) {
        return 0;
    }

    //get required substring
    std::string right = String.substr(slen - count);

    //copy substring to buffer
    strcpy_s(buffer, len, right.c_str());
    return slen;
}

[phorce]

To me, it appears that you are passing in too many variables. What do these do?

Another thing:

 //check input is not empty
    if (String == "") {
        return 0;
    }

Here, in your comments you say "check if a string is not empty" but, in actual fact, you're checking to see if a string is empty..

Why create a variable for slen? your function could just have:

return(String.length())

I really don't know what this function does, to me, for some strange reason, it performs so many different tasks, yet only returns the length of a string. Is this really accurate?

[Suzie999]

source is the string to work with
len is the size of the buffer passed
count is the amount of chars from right of string, to copy to buffer.

I calculate slen once so I do not need to do it several times.

The check you mentioned returns from the function without trying to copy to buffer, because there is no string to work with.

If I pass "this string" to the function with a count of 4, "ring" will be copied to buffer.

returning the length of passed string was just me testing, but after mentioning it I will instead return length of string copied to buffer.

Thanks for your comments.

[iamthwee]

Is this for learning purposes?

[mike_2000_17]

returning the length of passed string was just me testing, but after mentioning it I will instead return length of string copied to buffer.

Yes, you should follow conventions. Most functions like this return the number of characters (bytes) written on the destination buffer. At least, that's the C convention (the C++ convention is to return the iterator (pointer or otherwise) to the one-passed-last element written), and the C convention is what you need to follow if you are writing a C API function like that.

BTW, the reason that it is convenient to return the number of characters written (like sprintf does) is so that you can do things like this:

void manufactureComplexString(int someOption) {
  char buffer[1024];
  char* p_str = buffer;
  int consumed = 0;

  consumed = sprintf(p_str, "This is just a test.\n");
  p_str += consumed;

  if(someOption & FIRST_OPTION_VALUES) {
    consumed = sprintf(p_str, "First option with value = %d\n", someOption & FIRST_OPTION_VALUES);
    p_str += consumed;
  };

  if(someOption & SECOND_OPTION_VALUES) {
    consumed = sprintf(p_str, "Second option with value = %d\n", someOption & SECOND_OPTION_VALUES);
    p_str += consumed;
  };

  printf(buffer); // print result.
};

The point is, it creates a convenient way to chain operations without having to keep track of too much information. The C++ convention is far nicer, by the way, but hey, if you're stuck with C, you're stuck with C.

Returning the length written is also a convenient way to express that "nothing was done" in case of an error.

I just get the feeling that I'm missing some error checking and would like to hear any comments or tips if you have the time.

Well, to some extent, there are always error checks omitted from almost any code, because otherwise, some trivial pieces of code could get very hairy. For example, when you create string objects, they could fail to allocate the memory necessary and throw a bad-alloc exception. Very often in C++, we don't really check the bad-alloc exception because in most cases, there isn't much you can do about it besides terminating the application, which is gonna happen anyways if an exception is left uncaught.

But even without going as far as making paranoid error checks, you can minimize your exposure to these errors by minimizing the complexity of the function. For instance, you create two string objects here, the String and the right strings (which are too very terrible names for variables! Use clear and meaningful names for variables). You will notice that both of these are useless because you shouldn't need more than the source and destination buffers to do a straight copy from one to the other. The main reason you seem to be using std::string is for the length() function and the substr function, which are both very trivial to do in "C-style", which will by-pass the creation of these strings and thus, avoid the need for the potentially-throwing and certainly-superfluous dynamic allocations of memory.

Also, you should have a const on your source string char-type to be more "C-style C++", instead of straight up C. Then, the len parameter should be paired with the buffer that it pertains too, currently, it looks like it has to do with the source string, not with the buffer buffer.

As for your error checking, you have the right idea, except that your checks are either a bit redundant or ambiguous. You check that the source is empty, and if the length is less than count... seems redundant, no? You can, at the very least, combine that into one check.

With those things in mind, you would get the following:

unsigned int __stdcall sRightmost(const char* source, unsigned int count, char* buffer, unsigned int len) {

    // do the safety check before the expensive operations:
    if( len < count + 1 ) {
        return 0;
    };

    //get length once only
    unsigned int slen = std::strlen(source);

    //check count is < slen
    if (count > slen) {
        return 0;
    };

    //copy substring to buffer (no need for "safe" version:
    strcpy(buffer, source + (slen - count));
    return count;
};

That's pretty much it.

One last thing, it is generally better to keep the __stdcall within a #define such that you can change it later if needed:

#define MY_LIBRARY_API_FUNCTION __stdcall

unsigned int MY_LIBRARY_API_FUNCTION sRightmost(const char* source, unsigned int count, char* buffer, unsigned int len) {
  //...

[Suzie999]

Thanks mike_2000_17

That is exactly the kind of input I was hoping for.
For the few years I've been learning to code, It's just been a hobby really.
But now I want to learn good practice, and why to do things.

I really appreciate your time, and will study what you've advised and learned me.

Thank you.

[Suzie999]

I also want to answer your comments about naming and the triviality of manipulating c style strings.

To me the naming is natural, and the code will not be exposed to the user in this case. It's a dll to be used in other languages you see.

As for C-Style, ever since I started I've used the lowest level I could, but consistently been advised to use std::* stuff, which is now what brought me to this point (where my fonts have gone funny) :)

I know I could do this with C-Style strings confidently now, but It's time to move along.

I'm still sticking with win32 though for a few more years :).

Once again, thanks mike.