Welcome to Software Development on Codidact!
Will you help us build our independent community of developers helping developers? We're small and trying to grow. We welcome questions about all aspects of software development, from design to code to QA and more. Got questions? Got answers? Got code you'd like someone to review? Please join us.
Post History
Design/API Consider hiding all details of the String internals to the caller by implementing opaque type - How to do private encapsulation in C? (As it happens, that post contains a example take...
Answer
#1: Initial revision
**Design/API** - Consider hiding all details of the String internals to the caller by implementing opaque type - [How to do private encapsulation in C?](https://software.codidact.com/posts/283888) (As it happens, that post contains a example taken from another string handling class.) - The code responsible for allocation should also be responsible for freeing. Avoid interfaces where you return pointers to dynamically allocated memory, then expect the caller to clean that up - that's exactly how memory leaks are made. Instead leave memory allocation to the caller whenever possible. - Avoid `bool` as error information type. Ok so this is a simple little library here so it's OK in this context. But for more complex libraries, consider implementing an `enum` return code instead. So that the caller gets information about what exactly failed, rather than just "something failed". - Similarly, returning `-1` or `NULL` etc isn't very helpful either. Avoid mixing up error information with the data, if possible. The return type of the function is usually reserved for the error information and data should be passed through parameters. **Bugs** - `string_find` ought to both check for end of the array as well as the null terminator. You could just call `strchr` instead. Or otherwise, maybe implement it along the lines of this: for(int i=0; i<str->length; && str[i]!='\0'; i++) { if(str[i] == c) { return i; } } /* error handling here */ But again, better yet would be to implement some error information enum. - `for (i = 'a'; i <= 'z'; i++)` Strictly speaking, the letters of the alphabet are actually not guaranteed to be placed in sequence, so this loop could get stuck forever or add stuff that isn't letters to the string. It will work fine on mainstream implementations (ASCII, UTF8 etc) but fail on strange and exotic implementations (EBCDIC, extended locales etc). One possible fix: const char alpha[] = "abcdefghijklmnopqrstuvwxyz"; for(int i=0; i<26; i++) string_append_char(str, alpha[i]); **Best practices & coding style** - Use _const correctness_ on all parameters passed through pointers when the function should not modify that parameter. - I agree with the previous review that `if (str == NULL) return NULL;` can help clarity a lot, since deeply nested functions with many brace levels turns harder to read. **Standard compliance** - Although it is common to name header guards something like `#ifndef __STRING_H__` (totally guilty of that myself in the past), this is actually a reserved identifier for the compiler. Avoid any identifier beginning with an underscore, in particular double underscore or underscore followed by an upper-case letter.