4
\$\begingroup\$

I have implemented few functions in C language that manipulate strings. These functions are not present in the standard C library.


string_library.c


#include <stdio.h>
#include <stdlib.h>

#include "string_library.h"


/*
 * get_input_from_stdin_and_discard_extra_characters(char *str, long size):
 *
 * Function get_input_from_stdin_and_discard_extra_characters() reads at most
 * 'size - 1' characters into 'str' from stdin and then appends the NULL
 * character ('\0'). In all cases, reading input stops after encountering a
 * newline ('\n') or EOF even if 'size - 1' characters have not been read.
 * If a newline ('\n') or EOF is read then these are replaced by NULL character
 * ('\0'). If there are extra characters in input, they are read and discarded.
 * In all cases, str is returned.
 *
 */
char *get_input_from_stdin_and_discard_extra_characters(char *str, long size)
{

    char c = 0;
    long num_chars_to_read = size - 1;
    long i = 0;

    if (!str)
        return NULL;

    if (num_chars_to_read <= 0)
        return NULL;

    for (i = 0; i < num_chars_to_read; i++) {

        c = getchar();

        if ((c == '\n') || (c == EOF)) {
            str[i] = 0;
            return str;
        }

        str[i] = c;

    } // end of for loop

    str[i] = 0;

    // discard rest of input
    while ((c = getchar()) && (c != '\n') && (c != EOF));

    return str;

} // end of get_input_from_stdin_and_discard_extra_characters


int is_string_empty(const char *str)
{

    if (!str)
        return 1;

    if (*str == 0)
        return 1;

    return 0;

} // end of is_string_empty


int is_string_all_whitespace(const char *str)
{

    char *temp = (char *)(str);

    if (is_string_empty(str))
        return 1;

    while (*temp) {
        if ((*temp != ' ') && (*temp != '\t'))
            return 0;
        temp++;
    }

    return 1;

} // is_string_all_whitespace


/*
 * This function fills 'str' with 'len - 1' random characters and appends a null
 * byte at the end and returns 'str'.
 */
char *get_random_string(char *str, long len)
{

    int min_num = 32;
    int max_num = 126;
    int i = 0;

    for (i = 0; i < (len - 1); i++) {
        str[i] = (random() % (max_num - min_num + 1)) + min_num;
    }

    str[len - 1] = 0;

    return str;

}


/*
 * str_replace_chr(char *str, const char orig, const char new):
 *
 * Function str_replace_chr() replaces all occurrences of 'orig' character in
 * the string 'str' with the 'new' character. If 'str' is NULL then nothing is
 * done. In all cases, 'str' is returned.
 *
 */
char *str_replace_chr(char *str, const char orig, const char new)
{

    char *orig_str = (str);

    if (!str)
        return orig_str;

    while (*str) {
        if (*str == orig)
            *str = new;
        str++;
    }

    return orig_str;

} // end of str_replace_chr


/*
 * substr(const char *str, long start_index, long end_index):
 *
 * Function substr() allocates memory and returns a pointer to a string / character
 * array which is a substring of 'str' starting from index 'start_index' till
 * 'end_index' (inclusive). This substring is terminated by NULL byte at the end.
 * If 'str' is NULL or 'start_index' is less than 0 or 'end_index' is less than 0
 * or 'end_index' is less than 'start_index' then NULL is returned.
 *
 * The returned pointer points to a memory region containing the substring and this
 * memory region was allocated using malloc. So, it is the user's responsibility to
 * free the allocated memory.
 *
 */
char *substr(const char *str, long start_index, long end_index)
{

    char *substring = NULL;
    long length = 0;
    long i = 0;

    if (!str)
        return NULL;

    if ((start_index < 0) || (end_index < 0) || (end_index < start_index))
        return NULL;

    length = end_index - start_index + 1;

    substring = malloc(length + 1); // extra 1 byte for NULL byte
    if (!substring)
        return NULL;

    for (i = 0; i < length; i++) {
        substring[i] = str[i + start_index];
    }

    substring[i] = 0;

    return substring;

} // end of substr

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Would you show us the header too? I doubt it contains anything too controversial, but it's always helps, for a full review. \$\endgroup\$ Nov 25 '21 at 9:28
9
\$\begingroup\$

get_input_from_stdin_and_discard_extra_characters

    c = getchar();

c is a char, so we lose the ability to distinguish EOF here. We need to use int for that.

The whole function could be made much simpler: just use fgets(), then if the final character read was \n, overwrite it, else discard the rest of the line with scanf("%*[^\n]"),scanf("%*c").

is_string_empty

The name is very close to those reserved for standard library extension, but is legal because is is followed by underscore rather than a lower-case letter.

It would be better named as is_string_null_or_empty, as a null pointer is quite different from an empty string. It could be simplified, too:

#include <stdbool.h>

bool is_string_empty(const char *str)
{
    return !str || !*str;
}

is_string_all_whitespace

Don't cast away const like that. temp can be a pointer to const char.

I'm surprised that only space and tab are considered - why not everything that's isspace()?

This too could be much simpler:

#include <ctype.h>

bool is_string_null_or_all_whitespace(const char *s)
{
    if (!s) { return true; }
    for (;;) {
        if (!*s) { return true; }
        if (!isspace((unsigned char)(*s++))) { return false; }
    }
}

get_random_string

Is there any value in returning the pointer that was passed? Consider a return type of void instead.

The length should probably be a size_t rather than long.

Those min and max values suggest that you're assuming the target is using ASCII - if so, that assumption should be documented (and preferably tested so that where it's untrue, users get a compilation error rather than random behaviour).

Is there a good reason not to use the standard library rand() for this function?

#include <assert.h>

void get_random_string(char *str, size_t len)
{
    assert(' ' == 32);          /* This function assumes ASCII */
    static const char min_num = 32;
    static const char max_num = 126;

    while (len--) {
        *str++ = (char)(rand() % (max_num - min_num + 1) + min_num);
    }
    *str = '\0';
}

substr

If we passed the indices as size_t, we wouldn't need to check for negative values.

In modern C, we don't need to declare all our variables at beginning of scope. It's better to wait until we can initialise them.

We should use the standard memcpy() function to copy the contents. Good compilers will recognise the loop and replace with memcpy(), but it's easier to use the function than to write the loop correctly.

We'll get undefined behaviour if we attempt to copy off the end of the array. We could use strncpy() to finish when we reach a null character, or we could measure the length and check that at the start of the function.

I like the good handling for when malloc() returns a null pointer.

#include <string.h>

char *substr(const char *str, size_t start_index, size_t end_index)
{
    if (!str || !*str ) {
        return NULL;
    }
    const char *end = memchr(str, '\0', end_index);
    if (end) {
        end_index = end - str - 1;
    }
    if (end_index < start_index) {
        return NULL;
    }

    const size_t length = end_index - start_index + 1;

    char *const substring = malloc(length + 1); // include terminating null char
    if (substring) {
        memcpy(substring, str + start_index, length);
        substring[length] = '\0';
    }

    return substring;
}

Overall

I'm disappointed to see no unit tests of these functions. Including tests in the review helps us understand what's known to work, and often tells us about the anticipated use cases. I encourage you to include tests whenever possible!

\$\endgroup\$
7
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$
    – Mast
    Nov 30 '21 at 19:58
  • \$\begingroup\$ Toby, I have made following changes in my code according to your review: (1) I changed c from char to int. (2) I have done the simplification - I have now used return ((!str) || (!(*str)));. (3) I have changed my code and now I am using isspace(). (4) I have put in documentation that the target is ASCII. (5) I am planning to use C string library functions from now on (like memcpy(), strncpy(), etc.). \$\endgroup\$
    – Amit
    Jan 5 at 13:55
  • \$\begingroup\$ Toby, scanf("%*[^\n]"),scanf("%*c"): Will this work correctly if EOF is encountered without newline being encountered? \$\endgroup\$
    – Amit
    Jan 5 at 13:58
  • \$\begingroup\$ Toby, long vs size_t: long is my personal preference. long guards against negative numbers being passed by user by mistake and long is also very large to satisfy almost all user needs. I haven't seen any use case of an integer greater than LONG_MAX (9,223,372,036,854,775,807) when dealing with strings. Also, if the standard C library uses size_t, it doesn't mean that I should also use size_t. \$\endgroup\$
    – Amit
    Jan 5 at 14:02
  • \$\begingroup\$ Toby, Is there a good reason not to use the standard library rand() for this function?: The man pages say that random() is better. See the paragraph below NOTES section in this link: linux.die.net/man/3/rand. \$\endgroup\$
    – Amit
    Jan 5 at 14:03
3
\$\begingroup\$

substr

This function does not work as I expected it to. If I called it like in the following:

substr("abc", 1, 1);

I would have expected it to return an empty string "" and not "b".

You should try to limit your function to do a single task. substr allocates a new string and writes to it. You can leave the allocation up to the caller.

Suggested replacement:

int substr(char* dst, size_t size, const char* src, size_t start, size_t length)
{
    if (length >= size || start + length > strlen(src))
        return -1;
    memmove(dst, src + start, length);
    dst[length] = 0;
    return length;
}

This has two advantages over your implementation:

  1. You do not have to remember to free the results
  2. It can be used on pre-existing buffers.

Consider the following

struct ipaddress
{
    char part1[4];
    char part2[4];
    char part3[4];
    char part4[4];
};

int main()
{
    char* test = "196.168.332.556";
    struct ipaddress adr;
    substr(adr.part1, 4, test, 0, 3);
    substr(adr.part2, 4, test, 4, 3);
    substr(adr.part3, 4, test, 8, 3);
    substr(adr.part4, 4, test, 12, 3);
}

If you used your implementation, you would have had to do an extra strcpy and remembered to free the string.

You will also be able to use the same buffer for the source and destination like so:

int main()
{
    char test[] = "123ABCDEF456"; 
    substr(test, sizeof(test), test, 3, 6);
    printf("%s\n", test);
}

str_replace_chr

Here you could have used strchr to search for the occurrences of orig:

void str_replace_chr(char* str, const char orig, const char new)
{
    while (str = strchr(str, orig))
    {
        *str++ = new;
    }
}
\$\endgroup\$
17
  • \$\begingroup\$ Did you consider introducing the restrict keyword for the pointer arguments to your version of substr, to tell the compiler (and the user) that the destination won't overlap the source string? Was omitting them (and using memmove() rather than memcpy()) an intentional choice? Worth elaborating on in your answer... \$\endgroup\$ Nov 25 '21 at 16:17
  • 1
    \$\begingroup\$ @TobySpeight, I considered that it might be legal for the destination to overlap and that is why I used memmove instead of memcpy. Consider the following char test[] = "ABCD123"; substr(test, sizeof(test), test, 2, 4); \$\endgroup\$
    – jdt
    Nov 25 '21 at 16:22
  • \$\begingroup\$ Yep, makes sense. That looks like a reasonable use-case. \$\endgroup\$ Nov 25 '21 at 16:36
  • 1
    \$\begingroup\$ Actually, strchr() can be implemented more efficiently than the plain loop on many architectures, so it might have a small performance advantage, particularly when the string is long with few or no matches. \$\endgroup\$ Nov 25 '21 at 16:43
  • 1
    \$\begingroup\$ Maybe be explicit that closed-range indices are rare for good reason, and also say why you prefer and changed to index+length at least for this name. \$\endgroup\$ Nov 25 '21 at 23:38
3
\$\begingroup\$

Functional difference from fgets()

I'd expect get_input_from_stdin_and_discard_extra_characters() to perform like fgets() when there is no input. Instead get_input_from_stdin_and_discard_extra_characters(str, ...) returns str. fgets(str, ...) returns NULL.

Types

Size of buffer and array indexing is more idiomatic C as size_t than long. Even int has some precedent, but not long.

Use int c to save result from fgetc() to well distinguish the 257 different responses. Avoid infinite loops when char is unsigned and fgetc() returns EOF.

Pedantic: signed math overflow

Perform if (num_chars_to_read <= 0) before long num_chars_to_read = size - 1; to avoid signed math overflow.

Missing string_library.h

Documentation

External functions deserve documentation in the .h file. Consider the user may not have access to the .c file (nor want it). Documentation in the .c file explains implementation. Overall functionality, higher level, is for the .h.

Name space

string_library.c contains various functions that have no or little name connection to string_library.*. Consider a more uniform naming convention.

get_input_from_stdin_and_discard_extra_characters() is too long.

Pedantic: range

get_random_string(str,len) is UB when len <= 0.

WET ()

() not needed in char *orig_str = (str);.

NULL is for pointers

Rather then comment

// extra 1 byte for NULL byte
// use 
extra 1 byte for null character
// or
extra 1 byte for \0
// or
extra 1 byte for nul

NULL is the null pointer constant. It may be a pointer type.

null character is what the C spec uses.

'\0' is another good choice

nul is defined outside C in ASCII.

Well uniformly formatted

No noticed spelling/grammar issues

\$\endgroup\$
14
  • \$\begingroup\$ Chux, - """"get_random_string(str,len) is UB when len <= 0."""" - this is not unbounded because in the for loop, the condition 0 < some_negative_number will not be satisfied and the loop will exit. However, it will crash because of """"str[len - 1] = 0;"""". So, I will fix the code to check 'len' for negative values. \$\endgroup\$
    – Amit
    Nov 28 '21 at 17:32
  • \$\begingroup\$ Chux, """"Perform if (num_chars_to_read <= 0) before long num_chars_to_read = size - 1; to avoid signed math overflow."""" - I don't see how will there be an overflow. Can you please give an example? \$\endgroup\$
    – Amit
    Nov 28 '21 at 17:34
  • \$\begingroup\$ Chux, , I have intentionally avoided size_t because it is unsigned. If by mistake user passes -1 then size_t will lead to a crash because -1 is ULONG_MAX and so str[ULONG_MAX] will crash. Also, malloc() will return NULL because it won't get memory of size ULONG_MAX. \$\endgroup\$
    – Amit
    Nov 28 '21 at 17:57
  • \$\begingroup\$ @Amit "fgets() never returns NULL." C spec has "If end-of-file is encountered and no characters have been read into the array, the contents of the array remain unchanged and a null pointer is returned. If a read error occurs during the operation, the array contents are indeterminate and a null pointer is returned. Unclear what code you used that had "If the first character read is '\n' or EOF then ... fgets() will return an empty string". For some clarity, we are talking about the return value which is either a null pointer or a pointer to a string. \$\endgroup\$ Nov 28 '21 at 19:24
  • 1
    \$\begingroup\$ @Amit "avoided size_t because it is unsigned. If by mistake user passes ...." --> If the user makes a mistake, then all sorts of unavoidable problems occur. Using a signed type may give warnings when an proper size_t variable is passed as the size. size_t is the right size type to use for all object sizing and array indexing - neither too narrow nor too wide. This tends to be a holy war, so in the end, follow your group's coding standards. C lib tends to use size_t. \$\endgroup\$ Nov 28 '21 at 21:32

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.