Communities

Writing
Writing
Codidact Meta
Codidact Meta
The Great Outdoors
The Great Outdoors
Photography & Video
Photography & Video
Scientific Speculation
Scientific Speculation
Cooking
Cooking
Electrical Engineering
Electrical Engineering
Judaism
Judaism
Languages & Linguistics
Languages & Linguistics
Software Development
Software Development
Mathematics
Mathematics
Christianity
Christianity
Code Golf
Code Golf
Music
Music
Physics
Physics
Linux Systems
Linux Systems
Power Users
Power Users
Tabletop RPGs
Tabletop RPGs
Community Proposals
Community Proposals
tag:snake search within a tag
answers:0 unanswered questions
user:xxxx search by author id
score:0.5 posts with 0.5+ score
"snake oil" exact phrase
votes:4 posts with 4+ votes
created:<1w created < 1 week ago
post_type:xxxx type of post
Search help
Notifications
Mark all as read See all your notifications »
Code Reviews

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

71%
+3 −0
Code Reviews A simple implementation of a mutable String in C

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...

posted 7d ago by Lundin‭

Answer
#1: Initial revision by user avatar Lundin‭ · 2025-02-14T10:45:11Z (7 days ago)
**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.