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

Substring function in C

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

Problem

I am trying to implement substring in C. How efficient and standard is this code? I have a hard time understanding how C functions are usually meant to work, like how printf() returns an int or size_t.

size_t substring(char *destination, char const *string,  int start, int len) {
    // writes a substring to the destinaton starting at start index of string until end stepping by step
    // follows the same start at 0 off by one on the end format as Java substring method
    int substringLength = len;

    int stringLength = strlen(string);

    if (start > stringLength || len > stringLength || start = len) {
        fputs("start must be start < len", stderr);
        return -1;
    }

    memcpy(destination, &string[start], substringLength);

    if (destination[stringLength] != '\x0') {
        destination[stringLength] = '\x0';
    }
    return substringLength;
}

Solution

Argument confusion

If you were trying to replicate the java substring function, you should have had a start and end argument. As it is, you have a start and length argument, but you didn't handle the length argument correctly.
Bug 1

This line doesn't correctly check for the case of copying off the end of the source string:

if (start > stringLength || len > stringLength || start < 0 || len < 0) {


It needs to be:

if (start + len > stringLength || start < 0 || len < 0) {


Edit: If you want to check for integer overflow, you can also throw in || start + len < 0, and use size_t everywhere instead of int.
Bug 2

This check is unnecessary and can return an error incorrectly:

if (start >= len) {
    fputs("start must be start < len", stderr);
    return -1;
}


For example, if the string has 10 bytes, start is 8 and len is 1, this check will return an error.
Bug 3

When you terminate the destination string, you use the wrong index. It should be len, not stringLength. Also, I'm not sure why you do a check before you write to it:

if (destination[stringLength] != '\x0') {
    destination[stringLength] = '\x0';
}


should be:

destination[len] = '\0';


Also, '\0' is the standard way to write the null character. I've never seen it written as '\x0' before.
Unnecessary local variable

The variable substringLength is unnecessary because it is always the same as len. You can just use len instead.
Return value

The java substring function returns the string. Your function returns the length but I don't think that this will be useful to the caller. The reason you might want to return a string is so you can chain calls like this:

strcat(substring(buf, path, dir_start, dir_length), basename);

Code Snippets

if (start > stringLength || len > stringLength || start < 0 || len < 0) {
if (start + len > stringLength || start < 0 || len < 0) {
if (start >= len) {
    fputs("start must be start < len", stderr);
    return -1;
}
if (destination[stringLength] != '\x0') {
    destination[stringLength] = '\x0';
}
destination[len] = '\0';

Context

StackExchange Code Review Q#112688, answer score: 3

Revisions (0)

No revisions yet.