Skip to main content
added 55 characters in body
Source Link
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

This looks pretty competent code to me. I Good to see that you've included handling for tabs.

I only have minor points to raise.

* > #define NON_BLANK(ch) (ch != '\t' && ch != '\n' && ch != ' '):

If we include `<ctype.h>`, we should be able to use `isspace()` instead of this macro.  In any case, a function is preferred to a macro, especially a macro that expands its arguments more than once (we can't call this with an argument such as `*++p`, for example).
  • #define NON_BLANK(ch)   (ch != '\t' && ch != '\n' && ch != ' ') 
    

    If we include <ctype.h>, we should be able to use isspace() instead of this macro. In any case, a function is preferred to a macro, especially a macro that expands its arguments more than once (we can't call this with an argument such as *++p, for example).

  • #define IN_NON_BLANK_SEQ 1
    

    We only have two states, so we should use a boolean (with a better name than state). Assuming we have <stdbool.h>, then I'd replace int state = !IN_NON_BLANK_SEQ; with

       bool in_blank_seq = false;
    

    Actually, we might not need this at all - I think that when we have most recently seen whitespace, then index_non_blank is non-zero.

  • int column_pos = 0;
    int index_non_blank = 0;
    

    These two could be unsigned. But that might not matter, given the small range of values they ever hold.

  • #define TAP_STOP_DIST 5
    int get_next_tap_pos(int current_column) {
    

    Spelling - that should be tab. And what kind of terminal has tab stops every five characters? Pretty much every terminal (and terminal emulator) I've ever used has tab stops every eight characters, at least unless specifically changed.

  • #define LINE_MAX_SIZE 21
    

    Additional exercise - make this configurable, by command-line option.

This looks pretty competent code to me. I only have minor points to raise.

* > #define NON_BLANK(ch) (ch != '\t' && ch != '\n' && ch != ' ')

If we include `<ctype.h>`, we should be able to use `isspace()` instead of this macro.  In any case, a function is preferred to a macro, especially a macro that expands its arguments more than once (we can't call this with an argument such as `*++p`, for example).
  • #define IN_NON_BLANK_SEQ 1
    

    We only have two states, so we should use a boolean (with a better name than state). Assuming we have <stdbool.h>, then I'd replace int state = !IN_NON_BLANK_SEQ; with

       bool in_blank_seq = false;
    

    Actually, we might not need this at all - I think that when we have most recently seen whitespace, then index_non_blank is non-zero.

  • int column_pos = 0;
    int index_non_blank = 0;
    

    These two could be unsigned. But that might not matter, given the small range of values they ever hold.

  • #define TAP_STOP_DIST 5
    int get_next_tap_pos(int current_column) {
    

    Spelling - that should be tab. And what kind of terminal has tab stops every five characters? Pretty much every terminal (and terminal emulator) I've ever used has tab stops every eight characters, at least unless specifically changed.

  • #define LINE_MAX_SIZE 21
    

    Additional exercise - make this configurable, by command-line option.

This looks pretty competent code to me. Good to see that you've included handling for tabs.

I only have minor points to raise:

  • #define NON_BLANK(ch)   (ch != '\t' && ch != '\n' && ch != ' ') 
    

    If we include <ctype.h>, we should be able to use isspace() instead of this macro. In any case, a function is preferred to a macro, especially a macro that expands its arguments more than once (we can't call this with an argument such as *++p, for example).

  • #define IN_NON_BLANK_SEQ 1
    

    We only have two states, so we should use a boolean (with a better name than state). Assuming we have <stdbool.h>, then I'd replace int state = !IN_NON_BLANK_SEQ; with

       bool in_blank_seq = false;
    

    Actually, we might not need this at all - I think that when we have most recently seen whitespace, then index_non_blank is non-zero.

  • int column_pos = 0;
    int index_non_blank = 0;
    

    These two could be unsigned. But that might not matter, given the small range of values they ever hold.

  • #define TAP_STOP_DIST 5
    int get_next_tap_pos(int current_column) {
    

    Spelling - that should be tab. And what kind of terminal has tab stops every five characters? Pretty much every terminal (and terminal emulator) I've ever used has tab stops every eight characters, at least unless specifically changed.

  • #define LINE_MAX_SIZE 21
    

    Additional exercise - make this configurable, by command-line option.

Source Link
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

This looks pretty competent code to me. I only have minor points to raise.

* > #define NON_BLANK(ch) (ch != '\t' && ch != '\n' && ch != ' ')

If we include `<ctype.h>`, we should be able to use `isspace()` instead of this macro.  In any case, a function is preferred to a macro, especially a macro that expands its arguments more than once (we can't call this with an argument such as `*++p`, for example).
  • #define IN_NON_BLANK_SEQ 1
    

    We only have two states, so we should use a boolean (with a better name than state). Assuming we have <stdbool.h>, then I'd replace int state = !IN_NON_BLANK_SEQ; with

       bool in_blank_seq = false;
    

    Actually, we might not need this at all - I think that when we have most recently seen whitespace, then index_non_blank is non-zero.

  • int column_pos = 0;
    int index_non_blank = 0;
    

    These two could be unsigned. But that might not matter, given the small range of values they ever hold.

  • #define TAP_STOP_DIST 5
    int get_next_tap_pos(int current_column) {
    

    Spelling - that should be tab. And what kind of terminal has tab stops every five characters? Pretty much every terminal (and terminal emulator) I've ever used has tab stops every eight characters, at least unless specifically changed.

  • #define LINE_MAX_SIZE 21
    

    Additional exercise - make this configurable, by command-line option.