1
\$\begingroup\$
void chrrem (char arr[], size_t len, size_t pos)
{
    memmove(arr + pos, arr + (pos + 1), (len - pos) + 1);
}

It is supposed to be just fast. Erases the character pointed by pos (starting from 0)


Note that this function works with a statically allocated array like so:

char arr [256] = "Mystring";

// >>
chrrem (arr, strlen(arr), 0);
chrrem (arr, strlen(arr), 0);

printf(arr);

Output:

sring
\$\endgroup\$
  • 4
    \$\begingroup\$ If you need to call this in a loop, it's a mistake. Rather, you want to erase all of them as you go instead of repeatedly copying the tail. Your question would be more on-topic if it showed how you use this function. \$\endgroup\$ – o11c Jul 15 '15 at 21:58
  • \$\begingroup\$ @nhgrif That's how the function works for me though. I will clarify that it has to be a statically allocated array. \$\endgroup\$ – Edenia Jul 15 '15 at 21:59
  • 6
    \$\begingroup\$ @nhgrif I understand how it can be called, what matters for the review is how it will be called. \$\endgroup\$ – o11c Jul 15 '15 at 22:01
  • \$\begingroup\$ @o11c Perhaps write a review then. It's not the fault of the function itself is being used incorrectly though. \$\endgroup\$ – nhgrif Jul 15 '15 at 22:01
  • 9
    \$\begingroup\$ This question is being discussed on the meta. \$\endgroup\$ – nhgrif Jul 16 '15 at 0:42
18
\$\begingroup\$

I am not a programmer, and I know short, cryptic function and variable names are used in for historical reasons, but I'll still comment that I don't like it one bit. I'd name the function removeCharacter.


Given that the function is only intended to be used on c-strings, is the len parameter actually necessary? Can't we eliminate that parameter and get the len in the same way your context code demonstrates calculating a c-string's length?

void removeCharacter(char string[], size_t index) {
     memmove(string+index, string + index + 1, strlen(string) - pos + 1);
}

Now we can actually justify the function wrapping the memmove. It makes it easier to use because all we have to do now is pass our string and the index of the character we want removed.


Given the performance implications of moving memory like this, it might be a great idea to offer a companion function which allows the user to remove a range of characters.

void removeSubstring(char string[], size_t index, size_t length) {
    memmove(string+index, string+index+length, strlen(string)-index+1);
}

I think it'd certainly be worthwhile to have a Range struct which has an index and length member, and simply pass this struct as the second argument instead of the index and length arguments.


And of course, it's important to be sure your code is well documented so that users are clear on exactly what they're getting with the function.

Also, this seems like a pretty easy thing to unit tests for, so be sure to do that.

\$\endgroup\$
  • \$\begingroup\$ We could change strlen( string)-index to strlen(string+index), and then string and index only appear as string+index, so you could do away with the index argument and provide a function that removes just the first character, removeFirstCharacter say, and then your removeCharacter is removeFirstCharacter( string+index). Similarly for removeSubstring. \$\endgroup\$ – dmuir Jul 16 '15 at 16:43
  • \$\begingroup\$ removeFirstCharacter(char string[]) { removeCharacter(string, 0) } You could also do a removeLastCharacter(char string[]) function. removeLastCharacter(char string[]) { string[strlen(string)] = '\0' } \$\endgroup\$ – nhgrif Jul 16 '15 at 16:45
  • \$\begingroup\$ It would be nice if the function could determine that the pos is out of range (at least in debug builds, but preferably for all builds), and return an error. Not doing so is what makes so many C standard library functions so dangerous. You could mess things up pretty badly by passing in an out-of-range pos. \$\endgroup\$ – user1118321 Jul 18 '15 at 16:39
3
\$\begingroup\$

There are not many ways to optimize the function by speed.

So about style:

char arr[]
arr + pos

looks inconsistent. If you don't address it as array, then why bother?

Second, most string and mem functions have the string length limit at the end of the parameter list - strncpy, strncat, memcmp, ...so I would switch the position of len and pos.

Third, most string functions return a pointer to the "new" string. I would therefore return the pointer to the resulting string, instead of void. This gives a caller more flexibility and in case you decide to implement error-checking one day, you can actually return NULL. Of course, the function will be slightly slower then.

Finally, I don't like the idea of putting strlen() into the function, because the caller will not be able to benefit from the knowledge about the string.

char bla[] = "MyString";
chrrem(bla, sizeof(bla) - 1, 0);

and you don't need to loop through the string to find the end any longer.

\$\endgroup\$
3
\$\begingroup\$

You copy one more char than needed. len - pos gives the tail including the terminating \0.

For example, your first call is with len = 8 and pos = 0. You need to copy the last 7 characters + \0, in total 8.

Another example, if you called with pos 4 giving "MySting", you need to copy "ing", in total 4. With len = 8 and pos = 4, the result is len - pos giving 4.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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