Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Overall Impression

##Overall Impression ThisThis code would be difficult to maintain especially if someone else had to pick up where the original coder had left off. This due primarily to the use of macros, multiple goto's and global variables.

A second consideration is that as the program uses more memory to contain the buffer, performance might be impacted.

A third consideration also about the performance is that the program will perform better if it reads a large block of text from the input file and then processes that text using string or character manipulation rather then using charater based input.

Global Variables

##Global Variables EvenEven though the variables the global namespace is protected from the variables exitval, spaceword, keyword, buf and bufsize by the use of static, programming within the file is still using the variables as global variables. This makes the code harder to write, read and debug because without searching the whole program it's not clear where the variables are modified. Use local variables whenever possible, and pass necessary information into functions as needed.

Use of Macros

##Use of Macros It'sIt's clear why the code has a macro (CHECKBUF) in it, it is to reduce code repetition which is good, however, it would be better to use a function rather than a macro. One of the drawbacks of using macros is that they are very difficult to debug because the code in them is not expanded in the debugger. Another drawback is that they tend to hide things if memory allocation , goto's or exit() statements are in them, this code has 2 out of 3 of those hidden items in the macro.

Portability

##Portability TheThe C programming language is very portable as long as the C programming standard is followed, and not some other standard such as POSIX. Two of the header files in this code (err.h and unistd.h) are not portable to Windows with out additional work being done to port that code or the associated libraries.

More portable code would write error messages and warning messages to stderr and not use err(), warn() or errc(). You could write your own portable library that recreates these functions, it might be a very good learning experience that you could share here on code review.

Another library function you could consider writing because it isn't portable is getopt(). I think this might even be a better learning experience.

The Use of Goto's

##The Use of Goto's ThereThere is sometimes a need to use goto's in error handling code, but that is pretty rare. To use multiple goto's for flow control in a function is to return to the original versions of BASIC and FORTRAN which didn't have many of the modern programming constructs. This used to be known as speghetti code. Blocks of code can be nested inside if statements, if the blocks of code ae too large or complex they can become functions. In the C programming language the break; statement can be used to exit a logic block. In the case of the getword() function perhaps it would be better to have it call two function that process the text, one for the -k switch and one for the -w switch.

##Overall Impression This code would be difficult to maintain especially if someone else had to pick up where the original coder had left off. This due primarily to the use of macros, multiple goto's and global variables.

A second consideration is that as the program uses more memory to contain the buffer, performance might be impacted.

A third consideration also about the performance is that the program will perform better if it reads a large block of text from the input file and then processes that text using string or character manipulation rather then using charater based input.

##Global Variables Even though the variables the global namespace is protected from the variables exitval, spaceword, keyword, buf and bufsize by the use of static, programming within the file is still using the variables as global variables. This makes the code harder to write, read and debug because without searching the whole program it's not clear where the variables are modified. Use local variables whenever possible, and pass necessary information into functions as needed.

##Use of Macros It's clear why the code has a macro (CHECKBUF) in it, it is to reduce code repetition which is good, however, it would be better to use a function rather than a macro. One of the drawbacks of using macros is that they are very difficult to debug because the code in them is not expanded in the debugger. Another drawback is that they tend to hide things if memory allocation , goto's or exit() statements are in them, this code has 2 out of 3 of those hidden items in the macro.

##Portability The C programming language is very portable as long as the C programming standard is followed, and not some other standard such as POSIX. Two of the header files in this code (err.h and unistd.h) are not portable to Windows with out additional work being done to port that code or the associated libraries.

More portable code would write error messages and warning messages to stderr and not use err(), warn() or errc(). You could write your own portable library that recreates these functions, it might be a very good learning experience that you could share here on code review.

Another library function you could consider writing because it isn't portable is getopt(). I think this might even be a better learning experience.

##The Use of Goto's There is sometimes a need to use goto's in error handling code, but that is pretty rare. To use multiple goto's for flow control in a function is to return to the original versions of BASIC and FORTRAN which didn't have many of the modern programming constructs. This used to be known as speghetti code. Blocks of code can be nested inside if statements, if the blocks of code ae too large or complex they can become functions. In the C programming language the break; statement can be used to exit a logic block. In the case of the getword() function perhaps it would be better to have it call two function that process the text, one for the -k switch and one for the -w switch.

Overall Impression

This code would be difficult to maintain especially if someone else had to pick up where the original coder had left off. This due primarily to the use of macros, multiple goto's and global variables.

A second consideration is that as the program uses more memory to contain the buffer, performance might be impacted.

A third consideration also about the performance is that the program will perform better if it reads a large block of text from the input file and then processes that text using string or character manipulation rather then using charater based input.

Global Variables

Even though the variables the global namespace is protected from the variables exitval, spaceword, keyword, buf and bufsize by the use of static, programming within the file is still using the variables as global variables. This makes the code harder to write, read and debug because without searching the whole program it's not clear where the variables are modified. Use local variables whenever possible, and pass necessary information into functions as needed.

Use of Macros

It's clear why the code has a macro (CHECKBUF) in it, it is to reduce code repetition which is good, however, it would be better to use a function rather than a macro. One of the drawbacks of using macros is that they are very difficult to debug because the code in them is not expanded in the debugger. Another drawback is that they tend to hide things if memory allocation , goto's or exit() statements are in them, this code has 2 out of 3 of those hidden items in the macro.

Portability

The C programming language is very portable as long as the C programming standard is followed, and not some other standard such as POSIX. Two of the header files in this code (err.h and unistd.h) are not portable to Windows with out additional work being done to port that code or the associated libraries.

More portable code would write error messages and warning messages to stderr and not use err(), warn() or errc(). You could write your own portable library that recreates these functions, it might be a very good learning experience that you could share here on code review.

Another library function you could consider writing because it isn't portable is getopt(). I think this might even be a better learning experience.

The Use of Goto's

There is sometimes a need to use goto's in error handling code, but that is pretty rare. To use multiple goto's for flow control in a function is to return to the original versions of BASIC and FORTRAN which didn't have many of the modern programming constructs. This used to be known as speghetti code. Blocks of code can be nested inside if statements, if the blocks of code ae too large or complex they can become functions. In the C programming language the break; statement can be used to exit a logic block. In the case of the getword() function perhaps it would be better to have it call two function that process the text, one for the -k switch and one for the -w switch.

Source Link
pacmaninbw
  • 26.1k
  • 13
  • 47
  • 114

##Overall Impression This code would be difficult to maintain especially if someone else had to pick up where the original coder had left off. This due primarily to the use of macros, multiple goto's and global variables.

A second consideration is that as the program uses more memory to contain the buffer, performance might be impacted.

A third consideration also about the performance is that the program will perform better if it reads a large block of text from the input file and then processes that text using string or character manipulation rather then using charater based input.

##Global Variables Even though the variables the global namespace is protected from the variables exitval, spaceword, keyword, buf and bufsize by the use of static, programming within the file is still using the variables as global variables. This makes the code harder to write, read and debug because without searching the whole program it's not clear where the variables are modified. Use local variables whenever possible, and pass necessary information into functions as needed.

##Use of Macros It's clear why the code has a macro (CHECKBUF) in it, it is to reduce code repetition which is good, however, it would be better to use a function rather than a macro. One of the drawbacks of using macros is that they are very difficult to debug because the code in them is not expanded in the debugger. Another drawback is that they tend to hide things if memory allocation , goto's or exit() statements are in them, this code has 2 out of 3 of those hidden items in the macro.

##Portability The C programming language is very portable as long as the C programming standard is followed, and not some other standard such as POSIX. Two of the header files in this code (err.h and unistd.h) are not portable to Windows with out additional work being done to port that code or the associated libraries.

More portable code would write error messages and warning messages to stderr and not use err(), warn() or errc(). You could write your own portable library that recreates these functions, it might be a very good learning experience that you could share here on code review.

Another library function you could consider writing because it isn't portable is getopt(). I think this might even be a better learning experience.

##The Use of Goto's There is sometimes a need to use goto's in error handling code, but that is pretty rare. To use multiple goto's for flow control in a function is to return to the original versions of BASIC and FORTRAN which didn't have many of the modern programming constructs. This used to be known as speghetti code. Blocks of code can be nested inside if statements, if the blocks of code ae too large or complex they can become functions. In the C programming language the break; statement can be used to exit a logic block. In the case of the getword() function perhaps it would be better to have it call two function that process the text, one for the -k switch and one for the -w switch.

lang-c

AltStyle によって変換されたページ (->オリジナル) /