5
\$\begingroup\$

I'm new to C as in I've only studied it for a year and it's my first language. I still don't know how to handle real world code and so would like to see how a real world programmer would write up a program that can find and replace text in a text file.

It took me 3 days to figure this out and I've tried my best to write it to standards. If anyone can suggest a way to improve this code without resorting to advanced techniques such as vectors or data structures, that would be great.

Here is my attempt:

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

#define CUTOFF 100

void *malloc_safe(size_t size);                                     // Wrapper for malloc() that includes exiting the program upon failure of dynamic memory allocation
void *realloc_safe(void *ptr, size_t new_size);                     // Wrapper for realloc() that includes exiting the program upon failure of dynamic memory allocation
char *dynamic_input_fgets(void);                                    // Initiates an input sequence with fgets() and returns the user input as a string in dynamically allocated memory
void strcpy_0n(char *dest, const char *source, size_t num);         // Copies `num` number of characters from `source` into `dest`, but without copying the null-terminator into the destination.
void strshift(char *str, int num, int direction);                   // Shifts all characters in the null terminated string `str` by `num` indexes left or right depending on the integer `direction`. direction can only be 1(shift right) or -1(shift left).

int main(void)
{
    char file_loc[1024];
    char *find_str, *replace_str, *file_content_stream;
    FILE *fp;
    int i, cur_sizeof_malloc_str;

    printf("Find and Replace a string in a text file. \nEnter file location: ");
    fgets(file_loc, 1024, stdin);
    if (strrchr(file_loc, '\n') != NULL)
        *(strrchr(file_loc, '\n')) = '\0';

    fp = fopen(file_loc, "r+");
    if (fp == NULL)
    {
        printf("\nError opening file: Are you sure the file exists? ");
        return 5;
    }

    printf("\nEnter string to find: ");
    find_str = dynamic_input_fgets();

    printf("\nEnter string to replace: ");
    replace_str = dynamic_input_fgets();



    // import file contents into program memory
    file_content_stream = malloc_safe(CUTOFF + 5);
    cur_sizeof_malloc_str = CUTOFF + 5;
    *file_content_stream = 0;

    while (1)
    {
        char tempstore[CUTOFF] = {0};
        if (fgets(tempstore, CUTOFF, fp) == NULL)
        {
            if (feof(fp))
                break;

            printf("\n\nAbnormal program termination. File access function \"fgets()\" failed");
            return 126;
        }

        strcat(file_content_stream, tempstore);

        cur_sizeof_malloc_str = cur_sizeof_malloc_str + CUTOFF;
        file_content_stream = realloc_safe(file_content_stream, cur_sizeof_malloc_str);
    }
    fclose(fp);



    // find and replace action
    for (i = 0; ; i++)
    {
        char *occurrence = strstr(file_content_stream, find_str);
        if (occurrence == NULL)
            break;
        else
        {
            // turns out find and replace is easier said than done
            // case: replace string is smaller or equal to find string...
            {
                int length_find_str = strlen(find_str);
                int length_replace_str = strlen(replace_str);

                // reallocate to increase space for larger string if string is longer...
                if (length_find_str < length_replace_str)
                {
                    int offset = length_replace_str - length_find_str;

                    file_content_stream = realloc_safe(file_content_stream, strlen(file_content_stream) + offset + 5);
                    // this causes occurrence to become invalid, refind occurrence again...
                    occurrence = strstr(file_content_stream, find_str);
                    if (occurrence == NULL)
                        break;
                    strshift(occurrence + length_find_str, offset, 1);
                    strcpy_0n(occurrence, replace_str, length_replace_str);
                }
                else // replace_str is smaller or equal, so no need to reallocate...
                {
                    int offset = length_find_str - length_replace_str;

                    if (offset == 0)
                    {
                        strcpy_0n(occurrence, replace_str, length_replace_str);
                    }
                    else
                    {
                        strcpy_0n(occurrence, replace_str, length_replace_str);
                        strshift(occurrence + length_find_str, offset, -1);
                    }
                }
            }
        }
    }

    if (i == 0)
    {
        printf("No matches found. ");
        return 0;
    }
    else
    {
        fp = fopen(file_loc, "w");
        if (fp == NULL)
        {
            printf("\nError opening/creating file: Are you sure the file exists? ");
            return 5;
        }

        fputs(file_content_stream, fp);
        printf("\n\nFound and replaced %d match(es)", i);

        fclose(fp);
        return 0;
    }
}

void *malloc_safe(size_t size)
{
    void *addr = malloc(size);
    if (addr == NULL)
    {
        printf("\n\nAbnormal program termination. Call to dynamic memory allocation failed");
        exit(255);
    }
    else
        return addr;
}

void *realloc_safe(void *ptr, size_t new_size)
{
    void *addr = realloc(ptr, new_size);
    if (addr == NULL)
    {
        printf("\n\nAbnormal program termination. Call to dynamic memory allocation failed");
        exit(255);
    }
    else
        return addr;
}

char *dynamic_input_fgets(void)
{
    char *str = malloc_safe(CUTOFF + 5);
    int cur_sizeof_malloc_str = CUTOFF + 5;
    *str = 0;

    while (1)
    {
        char tempstore[CUTOFF] = {0};
        fgets(tempstore, CUTOFF, stdin);
        strcat(str, tempstore);

        if (strrchr(str, '\n') != NULL) // so it has a newline at the end which means user ended input
        {
            str[strlen(str) - 1] = '\0';
            break;
        }
        cur_sizeof_malloc_str = cur_sizeof_malloc_str + CUTOFF;
        str = realloc_safe(str, cur_sizeof_malloc_str);
    }

    return str;
}

void strcpy_0n(char *dest, const char *source, size_t num)
{
    size_t i;
    for (i = 0; i < num; i++)
    {
        *dest = *source;
        dest++;
        source++;
    }
}

void strshift(char *str, int num, int direction)
{
    int i = strlen(str);

    // strshift assumes there is enough allocated memory to support shift left/right movements and hence makes no checks on illegal memory access. Ensure you have allocated enough space in memory to support shift right and left.

    switch (direction)
    {
    case 1: // shift right by num indexes/chars
        for(; i >= 0 ; i--)
            str[i + num] = str[i];

        num--;

        for(; num >= 0; num--)
            str[num] = 178; // masking agent for debug, usually this shouldn't even be visible once you have overwritten it.
        break;

    case -1: // shift left by num indexes/chars
        for (i = -num; str[i + num] != 0; i++)
            str[i] = str[i + num];
        
        str[i] = str[i + num]; // copies the null terminator too, couldn't figure out how to put this into the for loop without messing up the program.
        break;

    default:
        printf("Secondary compile error: Illegal parameter int direction for function strshift(). Program aborting.");
        exit(255);
    }
}
```
\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

Good points made already, let me add a few more if I may. Before that, a small comment regarding:

I still don't know how to handle real world code and so would like to see how a real world programmer would write up a program that can find and replace text in a text file. ... If anyone can suggest a way to improve this code without resorting to advanced techniques such as vectors or data structures, that would be great.

Well, in the real-world, search & replace is much more involved than adding a couple of data-structures. Wikipedia summarizes it nicely I think, just for searching.

Anyway, back to reviewing.

Coding Style

int main(void)
{
    char file_loc[1024];
    char *find_str, *replace_str, *file_content_stream;
    FILE *fp;
    int i, cur_sizeof_malloc_str;
    ...

while (1)
{
    char tempstore[CUTOFF] = {0};
    if (fgets(tempstore, CUTOFF, fp) == NULL)
    {
        ...
        strcat(file_content_stream, tempstore);
        cur_sizeof_malloc_str = cur_sizeof_malloc_str + CUTOFF;
        file_content_stream = realloc_safe(file_content_stream, cur_sizeof_malloc_str);
    ...

// find and replace action
for (i = 0; ; i++)
{
    char *occurrence = strstr(file_content_stream, find_str);
    if (occurrence == NULL)
        break;
    else
    {
        {
            int length_find_str = strlen(find_str);
            int length_replace_str = strlen(replace_str);
    ...

There are lots of style guidelines, but when it comes to plain standard C (pretty rare in the real word) consider taking a minimalistic approach, ideally using common idioms and conventions.

Consider keeping your variable names short but descriptive, and only use underscores when naming functions. If need be, a short in-line comment can provide a brief hint.

Also consider defining each variable on its own line, and probably initialize them with default/fallback values.

These few and easily picked up suggestions all help in keeping the code clean and easily maintainable.

In the following sample I've also added uppercase comments to draw your attention to a few more things I think you should consider:

/* K&R INDENT STYLE (mostly) */

int main(void)
{
    char fname[MAX_FNAME] = {'\0'};     // absolute or relative path (c-string)
    char *fbuf = NULL;                  // file contents (c-string)
    char *tokfind = NULL;               // find token (c-string)
    char *tokrepl = NULL;               // replace-with token (c-string)
    FILE *fp = NULL;
    int allocsz = 32;                   // initial alloc-ahead size
    int i;
    ...

while (1) {
    char tmpbuf[CUTOFF] = {'\0'};
    if (fgets(tmpbuf, CUTOFF, fp) == NULL) {
        ...
        strcat(fbuf, tmpbuf);
        allocsz += CUTOFF;
        fbuf = realloc_safe(fbuf, allocsz);
    ...

// find and replace action
for (i=0; /* void */ ; i++) {
    char *cpmatch = strstr(fbuf, tokfind);  /* "cp" PREFIX IS char-pointer IDIOM */
    if (cpmatch == NULL)
        break;
    
    /* NO NEED FOR else AFTER if () {...; break;}
       (IT SAVES US 1 LEVEL OF INDENTATION) */
    
    /* WHY THE EXTRA SCOPE-NESTING HERE? */
    {
        /* SHOULD MOVE THESE OUTSIDE THE LOOP */
        int len1tokfind = strlen(tokfind);
        int len2tokrepl = strlen(tokrepl);
    ...

 

Function Prototyping

Function prototypes are quite useful in large projects, especially for providing public interfaces to private implementations, but in tiny 1-module programs like this, they are of little use (if any at all). They just clutter the code for no good reason.

Consider defining your functions in their calling order (with main() defined last) and remove their declarations (prototypes). Often this also helps in better organizing the logic of the program.

Either way, please document your function definitions (it doesn't have to be a long text). Right now they are all undocumented.

Re-inventing the Wheel

Avoid re-implementing already available well-tested functions, unless they really (really really really) don't cut it for you (that said, re-inventing the wheel can be an awesome learning-experience).

For example:

// Initiates an input sequence with fgets() and returns
// the user input as a string in dynamically allocated memory
char *dynamic_input_fgets(void);

Although the standard C library doesn't provide an equivallent function, the almost everywhere available, open-source function: getline() does exactly that, and more (btw, from a beginner's POV, using it properly is also a good learning-experience).

Error Handling

It's been already mentioned by G.Sliepen that for this program multiple exit codes don't make much sense (if any at all).

If you really have to, the standard C library provides the errno.h header, for basic error-handling. Consider using that instead.

For debugging purposes, you may also want to check out the assert.h header and the assert() macro. It's main purpose is to immediately exit on error, thus making it easier to find the cause.

In production code, errors are usually handled quite differently, with several approaches and mechanisms depending on the project, but they rarely exit immediately.

Instead of printing error-messages and exiting from within functions, consider making them return errors (either directly or via a parameter which can be anything, ranging from a simple type to a pointer to an error-handler mechanism).

This way you can have a centralized error-handler, responsible for messaging and further actions (it can be your main() for starters).

Btw, I see no good reason for having malloc_safe() and realloc_safe(). All they do is hiding error-checking one level deeper, and that's rarely a good thing. Just do your error-checking immediately after calling malloc() or realloc(), in the same scope.

Another point regarding errors:

printf("\nError opening file: Are you sure the file exists? ");

Consider directing all your error-messages to stderr instead of stdout:

fprintf(stderr, "Error opening file %s\n", fname);

It's not only good practice, but it can also help in separating normal output from error-messages via redirection (2 is the stderr stream id). See also this topic.

On a last note for this section, validate the results of functions before using them. For example:

printf("\nEnter string to find: ");
find_str = dynamic_input_fgets();

printf("\nEnter string to replace: ");
replace_str = dynamic_input_fgets();

Here you don't check the returned values of find_str and replace_str before using them later on. When the user enters here 2 empty strings, your program goes into an infinite-loop later on (alternatively you can catch the error then, but you don't do that either).

You could easily prevent that, by demanding valid input before accepting it. Something like this:

for (;;) {
    printf("\nEnter string to find: ");
    find_str = dynamic_input_fgets();
    if ( !find_str ) {
        /* fatal error: exit or direct flow to the error-handler */
    }
    // stop if valid
    if ( *find_str != '\0' ) {
        break;
    }
    // keep asking if invalid
    free( find_str );
    puts( "please type in at least 1 character" );
}

Being Consistent

Using the same example again:

printf("Find and Replace a string in a text file. \nEnter file location: ");
fgets(file_loc, 1024, stdin);
...
printf("\nEnter string to find: ");
find_str = dynamic_input_fgets();

printf("\nEnter string to replace: ");
replace_str = dynamic_input_fgets();

Consider being consistent across similar situations. In the above example, you have 3 c-strings which all can benefit from being dynamically allocated, but the 1st one is defined statically and read with fgets() while the last 2 are properly defined as dynamic and read with dynamic_input_fgets(). I see no good reason for that, especially having file_loc as a static c-string limited to 1024 bytes long. My guess would be some kind of issue with your custom dynamic_input_fgets() when it comes to file_loc, but if so, that could be another reason for using getline() instead of your custom one.

Consider the following alternative approach which stays consistent across all these 3 c-strings regarding their definition and inputting, and also summarizes most of my suggestions so far (it also uses some trivial custom functions like str_trim(), str_clear_eol(), file_exists(), etc, but the main point stays the same).

char *fname = NULL;                     // absolute or relative path (c-string)
char *tokfind = NULL;                   // find token (c-string)
char *tokrepl = NULL;                   // replace-with token (c-string)
size_t linesz = 0;                      // for passing it to getline()

// get & validate the filename
printf("Enter filename: ");
if ( -1 == getline(&fname, &linesz, stdin) ) {
    goto cleanup_and_fail;
}
str_trim( fname );
if ( !file_exists(fname) ) {
    fprintf(stderr, "No such file found\n");
    goto cleanup_and_fail;
}

// get & validate the find token
for (;;) {
    printf("Enter string to find: ");
    if ( -1 == getline( &tokfind, &linesz, stdin) ) {
        fprintf(stderr, "stdin read error, aborting!\n");
        goto cleanup_and_fail;
    }
    str_clear_eol( tokfind );
    if ( *tokfind != '\0' ) {
        break;
    }
    puts( "please type in at least 1 character, try again..." );
}

// get & validate the replace-with token
...

The goto statements used like that can help in cleaning-up before exiting (successfully or not) without cluttering the main body of the code. I'm addressing that in the Cleaning Up section, below.

Cleaning Up

Consider cleaning up your dynamically allocated memory before exiting (successfully or not). In some cases it doesn't matter, but in others it does (memory leaks). In any case, consider making it a habit to always clean up.

This is often easier said than done, but I really think you should deal with it if you are to use C seriously.

You don't do it in your code, but in this case it's easy to add it. In a nutshell you want to free() everything you have successfully malloc()/calloc()/realloc()'ed. That pretty much means your:

char *find_str, *replace_str, *file_content_stream;

and if you follow my suggestion and change file_loc to a dynamically allocated buffer, then that one too.

Instead of repeating the cleanup code at several exit points scattered around, you can gather it at the bottom of main() (or another error-handler) as following (btw, that's another reason of having a centralized error-handling scope, instead of exiting from within functions on error):

int main( void )
{
    // will be dynamically allocated
    char *fname = NULL;                     // absolute or relative (c-string)
    char *fbuf = NULL;                      // file contents (c-string)
    char *tokfind = NULL;                   // find token (c-string)
    char *tokrepl = NULL;                   // replace token (c-string)
    ...

    /* dynamic allocation of any or all of the above */
    ...

    if ( fatal-error ) {
        // whatever extra work needed
        goto cleanup_and_fail;
    }
    ...

    if ( conditional-normal-exit ) {
        goto cleanup_and_exit;
    }
    ...

cleanup_and_exit:
    free( fname );
    free( tokfind );
    free( tokrepl );
    free( fbuf );
    return EXIT_SUCCESS;

cleanup_and_fail:
    free( fname );
    free( tokfind );
    free( tokrepl );
    free( fbuf );
    return EXIT_FAILURE;
}

Hopefully, the sample code at the end of the Being Consistent section makes more sense now.

Anyway, that's a simplification of the general idea (in the real-word you often work with class-like instants, with their own destructors).

Scalability

If you plan to use the search & replace functionality in different parts of the program, say after adding more features (or even to other projects) consider making it a re-usable function.

For example, a function accepting a source c-string and returning the processed result along with supporting information.

Or a function that accepts a text-stream pointer to modify its contents.

Or any re-usable combination.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you for your lengthy discourse! First of all I'd like to say that I avoided freeing the allocated memory because it's a small application and closing the program frees the memory anyway. Your example codes definitely look very professional and feel as if it was part of something that was much larger in size. \$\endgroup\$ May 29, 2021 at 13:58
  • \$\begingroup\$ 2) My reasoning behind giving file_loc 1024 characters was because it's much simpler to have a static char array instead of a malloc-string and that it's very unlikely that a file location string will be larger than 1024 characters. It was my way of simplifying and making optimizations. There's no issue with dynamic_input_fgets() at all. \$\endgroup\$ May 29, 2021 at 14:09
  • \$\begingroup\$ 3) malloc_safe and realloc_safe are just function wrappers that behave in the same way as malloc/realloc (because they have the same parameters and return types) except that they check for NULL returns and perform error handling, because copying and pasting the same code multiple times in the program made it quite redundant. While your method of error handling is very elegant it is also quite hard for me and would require some getting used to (not something that I as a beginner would think of on my own) \$\endgroup\$ May 29, 2021 at 14:09
  • \$\begingroup\$ @AhnafAbdullah, sorry for the late response. 1) Yes, cleanup is not necessary here, and actually when it has lots to do it's often omitted on purpose to speed up termination. However it hurts scalability and one has to remember to do it after scaling up the project. 2) It is not that rare in our Unicode world, since files can be nested deep down in the path tree, with the naming of each node being controlled by the user. Btw, the find & replace tokens are way less likely to ever be longer than 1024 bytes, so if nothing else it would make more sense to have those fixed, instead of the filename. \$\endgroup\$
    – Harry K.
    Jun 4, 2021 at 11:10
  • 1
    \$\begingroup\$ @AhnafAbdullah, 3) Yeah I get why you did it, but my whole reply was a response to your original "how real-world programmers would do things" query. So, this is not how error-handling is done out there ;) \$\endgroup\$
    – Harry K.
    Jun 4, 2021 at 11:17
5
\$\begingroup\$

In addition to mangupt's excellent points, I would like to add:

Split up the code into more functions

Your main() is still quite large, and there are lots of opportunities to split off functionality into their own functions. This keeps main() smaller and easier to read and maintain. For example, consider the code that removes the newline character:

if (strrchr(file_loc, '\n') != NULL)
    *(strrchr(file_loc, '\n')) = '\0';

Even for just these two lines you can create a function:

static void remove_newline(char *line) {
    char *newline = strchr(line, '\n');
    if (newline)
        *newline = '\0';
}

Since this piece of code now has a proper name, it's much clearer what you are doing when you call it from main().

Avoid magic numbers

Avoid magic numbers, like 1024 for the size of file_loc. The first thing to do is to give them a proper name:

#define MAX_FILENAME_SIZE 1024

And then use the name instead of the number where appropriate. However, also think about why you chose this particular number. What is special about 1024? Are all filenames guaranteed to be less than 1024 characters? What happens if someone inputs a filename that is longer?

Other examples in your code are 5, 126 and 255 for the exit codes. Why did you pick those values? Does it matter for the caller that they get different codes because of different errors? If there is no good reason, I recommend you just use EXIT_SUCCESS and EXIT_FAILURE for the return codes.

Use memmove() and memcpy()

Your strshift() function is not necessary, the standard library provides you with memmove() which basically does the same thing. The strcpy_0n() blindly copies num bytes, so it can be replaced with memcpy().

Consider not reading in the whole file

There are several issues with reading in the whole file in memory, and then doing search/replace actions on it. First, consider that files might be much bigger than the amount of available memory. But more importantly, even if it fits in memory, calling strshift() or memmove() repeatedly might be very costly if the buffer is large. Even worse, if the replacement string is larger than the search string, you have to grow the buffer for every occurence of the search string.

I suggest you try to make it work by reading input into a small, fixed buffer. Scan for the string in that, if not found you just print the whole buffer. If it is found, print everything up to the start of the occurence of the search string, then print the replacement string, then continue search from right after the first occurence of the search string, until you reach the end of the buffer. Then read in the next part of the file and repeat. The only problem left is to properly handle the search string starting near the end of the buffer, so it was not fully read in, but that is also easily solvable.

Make your program work without needing interactive I/O

After starting your program, it asks you to enter the filenames and search and replacement strings. While this is fine if you just run the program manually once or twice, it becomes a problem if you want to use your program in a more automated fashion, like having another script call it to search and replace things. Even for interactive use, it's much nicer if you can specify everything on the command line, since shells typically have a history, globbing/tab completion for filenames and so on. I suggest you do the following things instead:

Read the search and replacement strings from the program's arguments

You can have the main() function read the arguments given on the command line when your program was started. Use this for the search and replacement string:

int main(int argc, char *argv[]) {
    if (argc <= 2) {
        fprintf(stderr, "Not enough arguments!\n");
        return EXIT_FAILURE;
    }

    const char *find_str = argv[1];
    const char *replace_str = argv[2];

    ...
}

Read the file from stdin and write the output to stdout

Instead of asking for filenames and opening the files yourself, just use stdin and stdout. On the command line you can use input redirection to ensure it reads from a given file and writes to another file. Combined with the above arguments for the search and replacement string, this would then look like:

./search_and_replace speling spelling < input.txt > output.txt

But even better, you can now make your program part of more complex commands, for example (my aplogies for the useless use of cat here):

cat input.txt | ./search_and_replace speling spelling | head -10

Note that input redirection is also working on Windows.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ strncpy is dangerous and should be avoided when possible. In particular, do not tell newbies to use it! See Is strcpy dangerous and what should be used instead? \$\endgroup\$
    – Lundin
    May 24, 2021 at 14:16
  • \$\begingroup\$ @Lundin Good point, and it wasn't even necessary to use strncpy() as a replacement, it was actually doing the same as memcpy(). I updated the answer. \$\endgroup\$
    – G. Sliepen
    May 24, 2021 at 16:50
  • \$\begingroup\$ Thanks for the review! 1) You should see my previous code bases lol... Back when I was new I'd write everything into the main function to the point where I'd copy and paste huge chunks of code... I've grown out of this odd behaviour now but I think making a function that's only two lines long and called only twice is a bit ridiculous 2) Excellent point about avoiding magic numbers, though I only use macros for meaningful numbers... 1024 is not a random number but its a number large enough to store a file name, and I hope the user will never find/replace a file larger in name than that... \$\endgroup\$ May 24, 2021 at 17:35
  • \$\begingroup\$ 3) I believe memmove() does not behave in the same way as strshift() and hence I avoided using those. I just needed to shift the contents in the string and writing down this function was a part of the problem-solving that allowed me to achieve the goals of this program (also, I prefer avoiding memory management functions cause they are quite hard to use...) 4) Input redirection is really hard for me, and since I am on Windows I avoided implementing that. For example ./search_and_replace speling spelling < input.txt > output.txt I have no idea what happens here or how this works... \$\endgroup\$ May 24, 2021 at 17:40
  • 1
    \$\begingroup\$ 1) Functions have the benefit that you can give them a name. Even if it's two lines of code that are only used once, you still get the benefit that the name will document what those two lines of code do. Very useful for others, and your future self when you're reading back your old code :) 4) If you start your program from cmd.exe, that line should cause stdin to read from input.txt and stdout to write to output.txt. See this manual. \$\endgroup\$
    – G. Sliepen
    May 24, 2021 at 20:51
3
\$\begingroup\$

I would not go into whether your program is working or not but I would like to give my views after seeing your code.

Pointer copy can be cleaned up:

void strcpy_0n(char *dest, const char *source, size_t num)
{
    ...
        *dest = *source;
        dest++;
        source++;
    ...
}

to

*dest++ = *source++;

fgets copies \0 at the end. So, no need to add \0 yourself. Same for every fgets in your code. Example: the following will read up to 1023 chars or \n and add \0.

fgets(file_loc, 1024, stdin);
if (strrchr(file_loc, '\n') != NULL)
    *(strrchr(file_loc, '\n')) = '\0';

Fixed type in *str = 0; Also, why add 5 chars? +1 is good enough.

...
file_content_stream = realloc_safe(file_content_stream, strlen(file_content_stream) + offset + 5);
...

char *dynamic_input_fgets(void)
{
    char *str = malloc_safe(CUTOFF + 5);
    int cur_sizeof_malloc_str = CUTOFF + 5;
    *str = '\0';

    ...
}

Reading a whole file is never good practice as the file can be huge. But if you want to read it all, don't malloc/realloc a huge number of times as this decreases performance. Instead use ftell to get the size of file, malloc once and read till feof.

while (!feof(fp)) {
    fgets(...)
}

Unnecessary re-calculation of fixed values and can be moved out of for (i = 0; …; i++):

int length_find_str = strlen(find_str);
int length_replace_str = strlen(replace_str);
int offset = length_replace_str - length_find_str;
\$\endgroup\$
9
  • \$\begingroup\$ Thanks a lot for your review! 1) First of all, I already know that fgets always returns a null-terminated string. The main purpose was actually to remove the newline character that results when you exit the input sequence by pressing Enter. Having the newline character at the end results in a fail on my system (Windows, mingw-w64) as the system cannot find the specified file. This makes sense, as the newline character is not a part of the file's name. 2) Fixed type in *str = 0; I didn't get what you meant by this... Can you please elaborate? \$\endgroup\$ May 21, 2021 at 12:15
  • \$\begingroup\$ 3) Nice catch with the unnecessary recalculations, now I'm looking at the code and thinking... 'Oh yeah, that IS unnecessary...' 4) About your opinion on the while (!(feof())), please look at this thread stackoverflow.com/questions/5431941/… Even before stackoverflow, I personally always had problems with logic when I tried while(!feof(fp)), so I personally abhor using this construct to stop the loop and prefer breaking instead. \$\endgroup\$ May 21, 2021 at 12:23
  • \$\begingroup\$ *str = 0 did you intend to have 0 or \0 \$\endgroup\$
    – mangupt
    May 21, 2021 at 15:59
  • \$\begingroup\$ they are literally the exact same thing though... \$\endgroup\$ May 21, 2021 at 17:25
  • 1
    \$\begingroup\$ *dest++ = *source++; is not necessarily an improvement. Yes it is commonly used but that doesn't make that line readable in itself - the only reason people can read it without trouble is because they have seen it before. Everyone else get a "WTF" moment. And since it doesn't lead to any efficiency improvement, there's no apparent need to use it. \$\endgroup\$
    – Lundin
    May 24, 2021 at 14:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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