Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

As C does not have a native string type, I decided to try making a similar type. It's not perfect by any means, so I was coming here to see what I could do to improve it.

I have tested my code and it works; if I were to write 'User', the output would then be 'Hello User'

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

struct string_t; //predeclare for access in str_node

typedef struct str_node {
    struct str_node* next; //next node in list
    struct string_t* mystr;//string associated
} s_node;

s_node* mhead = NULL;//head of node list

typedef struct string_t {
    char*  mybuf; //buffer
    size_t mycap; //capacity of string
} string; //definition of string

string* new_string()
{
    string* str = malloc(sizeof(string));//allocate a new string
    str->mybuf = NULL;//set the pointer to null so that it will not be deallocated if string is not used
    str->mycap = 0;//set the capacity to 0

    //add string to list of strings so that it can be deallocated in free_strings()
    s_node** nptr = &mhead;
    while ((*nptr) != NULL) {
        nptr = &((*nptr)->next);//point to the next node before free
    } //find the last node in the list
    (*nptr) = malloc(sizeof(s_node));//allocate a node
    (*nptr)->mystr = str;//set the string associated
    (*nptr)->next = NULL;//set the next node as null
    return (str);//return the new object
}

void fstr(string* str)
{
    //free a specific string
    if (str->mybuf != NULL) {
        free(str->mybuf);//free pointer if not null
    }
    if (str != NULL) {
        free(str);//free string object if not null
    }
}

void free_strings()
{
    //free all allocated strings and remove node list
    s_node* nptr = mhead;
    while (nptr != NULL) {
        //free the string
        fstr(nptr->mystr);
        s_node* next = nptr->next;//point to the next node before free
        free(nptr);//free the node
        nptr = next;//now point to the next
    }
}

size_t length(string* str)
{
    //if str->mybuf is null, return 0, otherwise strlen(str->mybuf)
    return ((str->mybuf == NULL) ? (0)
        : (strlen(str->mybuf)));
}

void ensure_capacity(string* str,
                     size_t cap)
{
    if (str->mybuf == NULL) {
        //allocate memory
        str->mybuf = malloc(cap);
        str->mycap = cap;
    }
    else if (str->mycap < cap) {
        //reallocate
        str->mybuf = realloc(str->mybuf, cap);
        str->mycap = cap;
    }
}

void trim(string* str,
          size_t len)
{
    //set null terminator at str->mybuf + len if necessary
    if (length(str) > len) {
        memset(str->mybuf + len, 0, 1);
    }//set null character at len
}

void set_length(string* str,
                size_t len)
{
    ensure_capacity(str, len + 1);//ensure capacity for len and null '\0'
    trim(str, len);//trim str to len characters if necessary
}

string* set(string* str,
            const char* value)
{
    //set the length, reallocates if appropriate
    set_length(str, strlen(value));
    strcpy(str->mybuf, value);//copy the string value
    return (str);//return str
}

string* append(string* str,
               const char* value)
{
    //set length, reallocate if appropriate
    set_length(str, strlen(value) + length(str));
    strcat(str->mybuf, value);//concatenate
    return (str);//return str
}

string* substring(string* str,
                  size_t idx,
                  size_t len)
{
    string* substr = new_string();//create a new string
    set(substr, str->mybuf + idx);//set the value of the string to the begin of the substring
    trim(substr, len);//trim the substring to len characters
    return (substr);//return the substring
}

void shrink_to_fit(string* str)
{
    //return unused memory to system
    size_t len = length(str) + 1;//ensure space for null character
    if (len < str->mycap) {
        //unused memory to return
        str->mybuf = realloc(str->mybuf, len);//reallocate to a smaller buffer
        str->mycap = len;//set capacity to smaller size
    }
}

string* read_from_n(FILE* stream,
                    size_t bufsize)
{
    string* buf = new_string();//allocate a new string to hold buffer
    ensure_capacity(buf, bufsize);//set a capacity of bufsize to hold input
    fgets(buf->mybuf, bufsize, stream);//get from stream into buffer
    shrink_to_fit(buf);//return spare memory, if any, to system
}

#define DEFAULT_READ_BUFFER_SIZE 1024

string* read_from(FILE* stream)
{
    //read_from_n using a defined buffer size
    return (read_from_n(stream, DEFAULT_READ_BUFFER_SIZE));
}

int main(void)
{
    string* s = new_string();
    set(s, "Hello World");
    string* sub = substring(s, 0, 6);
    append(sub, read_from(stdin)->mybuf);
    printf(sub->mybuf);
    getchar();
    free_strings();
}
share|improve this question
1  
there are several complications in the code that not necessary. However, these two lines are of a concern: 'if (str->mybuf != NULL) {' and 'if (str != NULL) {' if there is any possibility that parameter passed into the function 'fstr()' is NULL, then the first if() is accessing an offset, in memory slightly off of address 0. This is undefined behaviour and can lead to a seg fault event. and will corrupt the heap; when that value is low memory is passed to free(). Strongly suggest checking that the parameter is not NULL before doing anything. –  user3629249 Aug 26 at 20:38
1  
Always check (!=NULL) the returned value from malloc() (and family of functions) to assure the operation was successful, before using the returned value –  user3629249 Aug 26 at 20:42
1  
in general, never use the returned value from realloc() without first checking the returned value is not NULL. Otherwise, the original pointer is lost/overlayed, resulting in a memory leak. –  user3629249 Aug 26 at 20:47
1  
@user3629249, you are making good points here, why not turn these comments into another answer? I'm sure you can further elaborate on some of the things I've mentioned and also add new ones! ;) –  glampert Aug 26 at 20:56
1  
although stated otherwise, the code does not cleanly compile: 1) this line: 'printf(sub->mybuf);' causes the compiler to state: "warning: format not a string literal and no format arguments" 2) function: read_from_n() is set to return a pointer to a 'string', however it does not. this causes the compiler to state: "warning: control reaches end of non-void function" Strongly suggest fixing these problems. –  user3629249 Aug 26 at 21:04

1 Answer 1

up vote 7 down vote accepted

Implementation:

I don't really see why you would want to keep a global list of all strings in a normal program, so the existence of that feature is not obvious to me. But assuming you have a good reason for that, then why not make the linked list node part of the string type? That way you don't need to allocate memory in new_string twice. That also simplifies the code, since you don't have to manage two dynamically allocated blocks. string could be:

typedef struct string_t {
    char*  buffer;
    size_t capacity;
    struct string_t * next;
} string;

Side note: You can see that I've given the variables more proper names, so that your comments become obsolete. Good code with no comments is much better than bad code with lots of comments (more about this further down...).

Why not store the length of the string in the object itself? Getting the string's length is still a O(n) operation. One of the main advantages of this setup is being able to store additional metadata with the string chars themselves. I'd strongly suggest that you keep the length with the object, removing all those strlens.

The existence of void fstr(string* str) is questionable. It will free a string instance, but the global list of strings is not touched, thus leaving a deallocated pointer in that list. Is that the intended behavior, or did you forget to remove the deallocated string? BTW, the order of the check in the function is wrong! You first dereference a member variable, then check if str is not NULL. That function will crash with a null str pointer!

EDIT: Okay, now I see that fstr is used by free_strings, so I assume it was meant as an internal helper function. In that case, marking it static will make that clear to the reader. A static function has file-scope (AKA local). But there is still the need for a function to deallocate a single string, unlinking it from the list.

Comments:

Your commenting is problematic. 90% of them only describe what the code is doing.

string* str = malloc(sizeof(string));//allocate a new string

Yep, that goes without saying, every C programmer knows what malloc does.

Don't introduce visual pollution by describing the code, let the code describe itself, C is human readable, after all. You just have to pick meaningful names for functions and variables.

Use comments to tell the reader why something was done the way it was, use them to provide any additional information that will aid the understanding (i.e.: a formal definition of the problem, a link to some research paper).

I also suggest taking some time to read:

Naming:

You could work a bit more on the naming of functions. fstr, was already mentioned before and it should either be fixed or removed. If it is kept, free_string is the name for it. trim is actually truncating the string to a given length, so name it truncate. set is kind of vague, maybe set_text would be a bit better to keep it orthogonal with set_length.

Error checking:

Overall you do next to no error checking and parameter validation. This can actually be a serious issue as the code grows. One instance: you assume malloc/realloc never fail, but memory isn't infinite. You should handle the case of a NULL being returned.

Same goes for read_from_n. fgets might fail, the file might not be open, might be at EOF. Check the return value and handle a failure.

Parameter validation could also be better. At the very least, asserting that pointers are not null/counts are not zero, can greatly help you debug errors during development.

Miscellaneous:

You can avoid writing the type twice in a malloc call by using the named variable instead:

string* str = malloc(sizeof *str);

This makes the code slightly DRYer and facilitates renaming/changing the type if needed. Also note that the ( ) around sizeof are optional.

Some people add parenthesis to the return expression.

return (str);//return str

I personally find it bad, since return is not a function. This adds a lot of visual boilerplate to ordinary code.

share|improve this answer
    
I did consider unlinking the node from the list, but I thought that it would take too long to search through all of the memory to find the previous node as the list is not doubly-linked. That was why I used the fstr function in free_strings; (after being modified to if (str != NULL) { if (str->mybuf != NULL) { free(str->mybuf); }free(str);}) the node would not be erased from the list, but when fstr was called in free_strings, it would not free the string due to the first if not working, but would still free the associated node. –  Joe Aug 26 at 21:44
    
@Joe, think about the intrusive node (make it part of string). It should simplify things a bit... –  glampert Aug 26 at 22:22
1  
This answer could be improved with a link to this, but maybe I'm biased. ;) –  nhgrif Aug 26 at 23:32
1  
@nhgrif, thanks for sharing the link, I've added it to the answer. –  glampert Aug 27 at 0:07

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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