5
\$\begingroup\$

See the previous iteration: A trivial command line utility for trimming whitespace from lines in C

Note: see the next iteration at A trivial command line utility for trimming whitespace from lines in C - follow-up 2

Now my code looks like this:

#include <ctype.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #define HELP_MESSAGE "Usage: trim [-h] [-v] [FILE1, [FILE2, [...]]]\n" \ " -h Print the help message and exit.\n" \ " -v Print the version message and exit.\n" \ " If no files specified, reads from standard input.\n" #define VERSION_MESSAGE "trim 1.61\n" \ "By Rodion \"rodde\" Efremov 07.04.2015 Helsinki\n" #define HELP_FLAG "-h" #define VERSION_FLAG "-v" #define NEWLINE_CHAR '\n' /******************************************************************************* * This routine removes all leading and trailing whitespace from a string, * * doing that in-place. ( * ********************************************************************************/ static char* trim_inplace(char* start) { char* end; for (end = &start[strlen(start) - 1]; isspace(*end); --end) { *end = '\0'; } while (isspace(*start)) { ++start; } return start; } /******************************************************************************* * Implements a (singly) linked list node holding a single character. * *******************************************************************************/ typedef struct char_node_t { char c; struct char_node_t* p_next; } char_node_t; /******************************************************************************* * Extracts a string from a linked list of characters, and frees the memory of * * the list holding the characters. * *******************************************************************************/ static char* build_string(char_node_t* p_head, size_t length) { char* string = (char*) malloc(sizeof(char) * length); size_t i = 0; char_node_t* p_node; while (p_head) { string[i++] = p_head->c; p_node = p_head; p_head = p_head->p_next; free(p_node); } string[i] = '\0'; return string; } /******************************************************************************* * Gets a line from file of any feasible length (fitting into the RAM). * * Returns NULL if file is exhausted. * *******************************************************************************/ static char* my_getline(FILE* file) { char_node_t* p_head = NULL; char_node_t* p_tail = NULL; char_node_t* p_tmp; size_t string_length = 0; char current_character; if (feof(file)) { return NULL; } for(;;) { current_character = fgetc(file); if (current_character == NEWLINE_CHAR || current_character == EOF) { if (string_length == 0) { // Otherwise a zero-length string will leak and produce a // superfluous empty line after the end of a file. return NULL; } // +1 for the NULL terminator. return build_string(p_head, string_length + 1); } if (string_length == 0) { // Initialize list. p_head = p_tail = (char_node_t*) malloc(sizeof(char_node_t)); p_head->c = current_character; p_head->p_next = p_tail->p_next = NULL; } else { // Append one character node to the list. p_tmp = (char_node_t*) malloc(sizeof(char_node_t)); p_tmp->c = current_character; p_tmp->p_next = NULL; p_tail->p_next = p_tmp; p_tail = p_tmp; } ++string_length; } } /******************************************************************************* * Processes a file. * *******************************************************************************/ static void process_file(FILE* file) { char* line; while ((line = my_getline(file))) { puts(trim_inplace(line)); free(line); } fclose(file); } /******************************************************************************* * Prints the help message and exits. * *******************************************************************************/ static void print_help() { printf(HELP_MESSAGE); exit(EXIT_SUCCESS); } /******************************************************************************* * Prints the version string. * *******************************************************************************/ static void print_version() { printf(VERSION_MESSAGE); exit(EXIT_SUCCESS); } /******************************************************************************* * Checks the flags. * *******************************************************************************/ static void check_flags(int argc, char** argv) { size_t i; for (i = 1; i < argc; ++i) { if (strcmp(argv[i], HELP_FLAG) == 0) { print_help(); } else if (strcmp(argv[i], VERSION_FLAG) == 0) { print_version(); } } } /******************************************************************************* * The entry point for a trivial line trimmer. * *******************************************************************************/ int main(int argc, char** argv) { size_t i; FILE* file; check_flags(argc, argv); if (argc < 2) { process_file(stdin); return EXIT_SUCCESS; } for (i = 1; i < argc; ++i) { file = fopen(argv[i], "r"); if (!file) { perror("Error opening a file"); return (EXIT_FAILURE); } process_file(file); } }

I have fixed the following issues:

  • File handles are closed as soon as possible.
  • trim_inplace simplified.
  • No superfluous consts.
  • The program should handle lines of arbitrary length.
  • No return EXIT_SUCCESS at the end of main.

Is there anything else to improve?

\$\endgroup\$

2 Answers 2

Reset to default
4
\$\begingroup\$

Your program looks good to me, nice structure and coding style. A few other points that you might consider changing:

In C, you shouldn't cast from void* to T*:

A void pointer converts implicitly to any other pointer type in C, so you shouldn't cast the return value of functions like malloc, realloc and calloc. Doing so only complicates maintenance and increases the number of times you have duplicate types through the code. Read this discussion on SO for more about this. The only exception to this would be if you need to compile the code as C++, since in C++ this implicit conversion is disallowed.

Prefer using sizeof(*var) over sizeof(Type) when possible:

Again this applies to your malloc calls. Instead of writing code like this:

p_tmp = (char_node_t*) malloc(sizeof(char_node_t));

You should prefer using the variable itself as argument to sizeof. Repeating the type will make things harder if you ever need to change it. More places that must be fixed, and if you forget one, you might be allocating an incompatible amount of memory. If you use the variable itself, no worries:

p_tmp = malloc(sizeof(*p_tmp));

It should be noted that for this style you must deref p_tmp, otherwise, you would be allocating a pointer, not a struct.

sizeof(char) is always 1:

C99 requires sizeof(char) (and any signed, unsigned variant) to be always 1, so when allocating a char string, you can simply:

char * str = malloc(length_including_null_terminator);

Your named string constants seem like overkill:

HELP_MESSAGE and VERSION_MESSAGE are only used in one place, so you could very well throw them away and place the text directly in the printf() call. It would already be nicely wrapped into a function (i.e.: print_help, print_version), so changing the text would be very easy and would only involve changes to a single location is the source. You only really need named constants when you will have to replicate a value in several places or when you want to give a raw value some explanatory name. There would be no point in, for example, a constant like #define FORTY_TWO 42. Which leads us to NEWLINE_CHAR. I can see some reasons for defining it, if you ever need to change, but are there any systems where the new line char is not '\n'? Sure Windows uses \r\n, but then you would need a NEWLINE_STRING for that. Since this constant is also only used once inside my_getline(), I would throw it away an use the char literal until (if) there is an actual need for a named constant.

Declared variables closest to the first usage as possible:

Unless you aim for compatibility with some C standard older than C99, declared for loop counter variables inside the for:

for (int i = 1; i < argc; ++i)

Same is valid for other variables. A variable declared at the top of a function is like a mini-global. Reduce scope by declaring them were they are first needed.

The _t suffix is reserved for future use by POSIX:

The POSIX standard reserves identifiers ending in _t for future expansion (see here). So perhaps you should consider another name for char_node_t if you aim for strict compatibility with this Standard.

\$\endgroup\$
1
  • \$\begingroup\$ Actually, on Windows the newline char is still '\n' -- C does a bunch of magic behind the scenes to translate it to what Windows/DOS require. \$\endgroup\$ Commented Sep 10, 2019 at 2:00
3
\$\begingroup\$
  • Good use of constants in one place.
  • Please recheck your trim_inplace() function. It fails (walks off the end of the buffer) if the line consists solely of spaces (including empty sring).
  • Even if you fix that bug, trim_inplace() is rather inefficient. It makes three passes over the string. Writing a state machine would let you do it in one pass.
  • Your char_node_t wastes a ton of space. For each character in the string, you allocate a struct consisting of 1 char + 1 pointer + whatever padding the compiler adds + whatever padding malloc() adds + the global memory table. It would be much more space-efficient to allocate a whole buffer at once at the expected size, and if you overflow, double the size and reallocate.
\$\endgroup\$
2
  • \$\begingroup\$ How should I implement a state machine? Ignoring the leading whitespace is clear to me, yet I don't know how to deal with the trailing whitespace. \$\endgroup\$ Commented Apr 8, 2015 at 9:38
  • \$\begingroup\$ The three states are: in leading whitespace, not in whitespace, in non-leading whitespace. Transitions are obvious. Whenever you transition to a non-leading whitespace, save the current index. If you reach the end of the string and are in non-leading whitespace, then it was trailing whitespace and you know the position. \$\endgroup\$ Commented Apr 9, 2015 at 2:11

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.