Read a helpfiles textfile or potentially delete one if given an argument that's in the list

Question

I'm working on a MUD (multiplayer text game), and the helpfile system adds missing helpfiles (ones that are looked for but don't exist) in the following format, including the room number, character name, and desired helpfile. I tried to be as efficient as possible, but I'm not very experienced with C file io, and the overall code feels unwieldy.

[   42] Legolas: Isengard
[    9] Frodo: the One Ring
[  666] Sauron: Doooooom

I've expanded #defines for simplicity.

  int delete_helpfile_from_list(FILE *fhelp, char *helpfile) {
    if (!strlen(helpfile))
      return FALSE;
    
    FILE *ftemp = fopen("../data/help_temp.txt", "w");
    if (!ftemp) {
      bug("helpfiles: Unable to write to help_temp.txt", 0);
      return -1;
    }

    const size_t MAX_LINE_LENGTH = 50;
    char buf[MAX_LINE_LENGTH], line_helpfile[32];
    int found = FALSE;
    while (fgets(buf, MAX_LINE_LENGTH, fhelp)) {
      size_t len = strlen(buf);
      int match = FALSE;

      // Trim newline and trailing whitespace
      if (len > 0 && buf[len-1] == '\n')
        buf[--len] = '\0';
      while (len > 0 && isspace(buf[len-1]))
        buf[--len] = '\0';

      // Move a pointer from the end until it reaches the colon, then move back to the first letter
      char *ptr = buf + len - 1;
      while (ptr >= buf && *ptr != ':')
        ptr--;
      ptr += 2;

      // Copy the last word as the line's helpfile
      strncpy(line_helpfile, ptr, buf+len-ptr);
      line_helpfile[buf+len-ptr] = '\0';

      match = !str_cmp(line_helpfile, helpfile);
      if (match && !found)
        found = TRUE;
      else if (!match) {
        strncat(buf, "\n", sizeof(buf) - strlen(buf) - 1);
        fputs(buf, ftemp);
      }
    }

    fclose(ftemp);

    if (found) {
      if (remove(HELP_FILE)) {
        bug("helpfile: Can't delete help.txt", 0);
        return -1;
      }

      if (rename("../data/help_temp.txt", HELP_FILE)) {
        bug("helpfile: Can't rename help_temp.txt to help.txt", 0);
        return -1;
      }
    }

    return found;
  }

  void do_helpfiles(CHAR_DATA *ch, char *argument) {
    if (!is_staff(ch)) {
      do_function(ch, &do_help, argument);
      return;
    }

    FILE *fhelp = fopen(HELP_FILE, "r");
    if (!fhelp) {
      send_to_char("Unable to read helpfiles list.\r\n", ch);
      bug("helpfiles: Unable to read help.txt", 0);
      return;
    }

    char arg[MSL], helpfile[MSL];
    argument = one_argument(argument, arg);
    strcpy(helpfile, argument);

    if (!str_cmp(arg, "done") || !str_cmp(arg, "clear")) {
      int found = delete_helpfile_from_list(fhelp, helpfile);
      if (found == -1)
        send_to_char("Unable to add missing helpfile to list.\r\n", ch);
      else if (found)
        printf_to_char(ch, "Great work! Removing all instances of %s from the helpfile list.\r\n", helpfile);
      else
        send_to_char("Great work on finishing a new helpfile! It currently wasn't on the list.\r\n", ch);
    }

    else {
      char *buf;
      long flen;
      fseek(fhelp, 0, SEEK_END);
      flen = ftell(fhelp);
      fseek(fhelp, 0, SEEK_SET);
      buf = (char*)malloc(flen+1);
      size_t nread = fread(buf, 1, flen, fhelp);
      buf[nread] = '\0';

      if (!nread) {
        send_to_char("Unable to read helpfiles list.\r\n", ch);
        bug("helpfiles: Unable to read help.txt", 0);
      }
      else
        page_to_char(buf, ch);
    }

    fclose(fhelp);
  }

Don't expand macros "for simplicity". Review is most effective if you present the code as you would read and modify it. It's too late to change the code here, as it has already been reviewed, but please bear that in mind for future review requests. — Toby Speight
– Toby Speight, Commented Dec 16, 2024 at 16:56

chux · Accepted Answer · 2024-10-15 16:45:21Z

Save time. Enable all compiler warnings

This saves OP and reviewers time.

Missing tests of return values

Robust code promptly detect failings. Code does many, yet deserves a few more.

      flen = ftell(fhelp);
      if (flen == -1) error();
      if (fseek(fhelp, 0, SEEK_SET)) error();
      buf = (char*)malloc(flen+1);
      if (buf == NULL) error();

Since flen is a long and may exceed size_t range, check for potential overflow

  if (flen < 0 || flen >= SIZE_MAX) error();
  buf = malloc((size_t)flen+1);

Use bool, true, false rather than FALSE, TRUE

Use standard bool when possible.

int delete_helpfile_from_list(FILE *fhelp, char *helpfile) { deserve document to explain the possible return values and their meaning for the caller.

Missing include files

Many missing includes, defines and declarations:

#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define FALSE 0
#define TRUE 1
#define CHAR_DATA char
#define MSL 42 /* some value */
#define HELP_FILE "tbd.help"
extern void bug(const char *s, ...);
extern int str_cmp(const char *a, const char *b);
...

Simplify

No need to walk the entire string to find if it is "".

// if (!strlen(helpfile))
if (helpfile[0] == '\0')
  return FALSE;

is...() expect unsigned char values (or EOF)

isspace(buf[len-1]) is UB when buf[len-1] < 0. Better as ((unsigned char *)buf)[len-1] or the like.

Scary code

strncpy(line_helpfile, ptr, buf+len-ptr); line_helpfile[buf+len-ptr] = '\0'; needs a fair amount of review to determine if a proper string was made.

Perhaps: sprintf(line_helpfile, sizeof line_helpfile, "%s", ptr);?

Improved test, message

Rather than only test for 0, test if not all read.

size_t nread = fread(buf, 1, flen, fhelp);
...
//if (!nread) {
//        send_to_char("Unable to read helpfiles list.\r\n", ch);
if (nread != flen) {
        send_to_char("Unable to read entire helpfiles list.\r\n", ch);

Maybe add file name and length read to message?

Unneeded code

if (len > 0 && buf[len-1] == '\n') buf[--len] = '\0'; not needed when followed by while (len > 0 && isspace(buf[len-1])) buf[--len] = '\0';

Toby Speight · Accepted Answer · 2024-12-16 16:59:58Z

Here's a bug:

  buf = (char*)malloc(flen+1);
  size_t nread = fread(buf, 1, flen, fhelp);

When malloc() fails, it returns a null pointer. It's undefined behaviour to pass a null pointer to fread(), so we must check for that in between these two statements (I recommend changing the function signature so that we can return an error indication).

As an aside, malloc() returns a pointer to void, which is convertible to any object pointer in C without a cast. So writing (char*) is just clutter for the reader. Plain old char *buf = malloc(flen+1) is fine and preferable.

Here's another bug:

      while (len > 0 && isspace(buf[len-1]))

It's a very common error to pass a char to isspace() and other <ctype.h> functions, but it actually takes a positive int (or EOF), so we need to convert to unsigned char before the value is widened. Otherwise, we risk passing negative values, which results in undefined behaviour.

kiner_shah · Accepted Answer · 2024-10-25 10:54:58Z

You can include <stdbool.h> and use true/false instead of defining any macro like FALSE.
The function delete_helpfile_from_list can accept helpfile as const char*.
You need to modularize your code by introducing more functions. For example the below code:
```
// Trim newline and trailing whitespace
  if (len > 0 && buf[len-1] == '\n')
    buf[--len] = '\0';
  while (len > 0 && isspace(buf[len-1]))
    buf[--len] = '\0';
```
can be made into an independent function called strip() which would accept a buffer and return a trimmed buffer (or trim it in place).

Similarly, for parsing a line, you can create functions which would extract information from the line and fill a struct, for example.
Use meaningful names. For example, not sure what is MSL in do_helpfiles().
memset buffers to 0, else sometimes you may see weird output due to garbage values.

Stack Exchange Network

Read a helpfiles textfile or potentially delete one if given an argument that's in the list

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Read a helpfiles textfile or potentially delete one if given an argument that's in the list

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions