C implementation of a string

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
#include
#include

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

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:

-
What are examples of comments that tell you why instead of how or what?

-
@nhgrif also has a good post on his blog that nails the issue in a couple paragraphs: Writing Readable Code.

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.

StackExchange Code Review Q#101999, answer score: 7