Hot answers tagged

37

I see some things that I think could help you improve your code. Decompose your program into functions All of the logic here is in main in one rather long and dense chunk of code. It would be better to decompose this into separate functions. Check return values for errors The call to scanf can fail. You must check the return values to make sure they ...


22

Efficient file I/O By default, files opened with fopen() are buffered, meaning that not every call to fread() or fwrite() will result in a system call. Instead, the C library has an internal buffer and will try to read and write larger chunks at a time. However, you are still paying for the overhead of a regular function call each time you call fread() and ...


22

log is already declared in <math.h>. You don't need to declare it yourself. In fact, it could be harmful. As stated in another answer, do not use floating point math. In fact, you don't need to know the exact position of the leftmost bit. For your purposes, the value of codepoint is enough. For example, bitPos <= 7 is equivalent to codepoint < (...


21

typedef enum { EQUAL = 0, LHS_NEWER, RHS_NEWER } ECOMPARISON; A common convention for three-way comparisons is that of strcmp() - return any negative value if the first argument is less than the second, zero if equal, and positive if greater. If we take that approach, and if we can ensure that the version values are small enough to avoid ...


21

The code you've written is more or less a state machine, written the way that one might be constructed in assembly language. A technique like that technically works, but it doesn't scale well and you can wind up with problems that are extremely hard to debug. Your code only needs a small tweak to use the more traditional C-language way to implement a state ...


20

The goto debate is ancient, from the year 1966 when Edgar Dijkstra came up with a famous paper called "Go To Statement Considered Harmful". This was controversial and the debate is still going on to this day. Still, most of his conclusions are also valid to this day and most uses of goto is considered harmful spaghetti programming. However, there's ...


19

int letter is not a letter. When printing you call it The number. Name it accordingly: int number. The condition (1 <= letter && letter <= 26) == false is very hard to follow. As a general rule, avoid boolean constants in conditions. Rewriting it as: !(1 <= letter && letter <= 26) immediately calls for a deMorgan transformation ...


19

Use a 2D array instead of 6 1D arrays (vs. character0, character1, ...) Use a 1D array for n instead of (n0, n1, ...) Add EOS termination to the string that gets passed to strtol Here's a refactored version: bool checksum(char card[]) { char chars[6][3]; long nx[6]; for (int col = 0; col < 6; ++col) { int lo = col << 1; ...


18

Welcome to Code Review, we wish you the best. General Observations Congratulations on getting this to work, one function that is 356 lines long and almost 17K of text is a bit large and very hard to code and debug. Complexity The function main() is too complex and should be broken into functions. A general rule of thumb in all programming is that a function ...


17

Keep names and enum values in sync enum algs and alg_names[] are declared quite a bit apart from each other. That makes it hard to spot if they are in sync. It is best to declare the enum and the array with the names right next to each other, and use designated initializers to ensure the compiler will enforce the correct order, like so: enum algs { FCFS, ...


17

Overall Observations An interpreter should be able to read from standard in as well as from a file, this would break the entire tape model. The user could also redirect an input file to standard in. If you are going to program in C then you need to get comfortable with pointers. In the case of file input I would use an algorithm that reads a line at a time, ...


17

I recommend some additional compiler warning options: gcc -std=c17 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wstrict-prototypes -Wconversion 252015.c -o 252015 252015.c:22:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes] ll_LinkedList* ll_create (); ^~~~~~~~~~~~~ 252015.c:60:16: ...


16

It's a good idea for worker.c to include "worker.h", preferably as the very first line. That ensures that the declarations are consistent and that the header has no missing includes of its own. (So, in task.c move the task.h include to the top spot, and similarly with utils.h in utils.c.) I recommend adding include-guards on all your header files. I know ...


15

On the whole this is nicely done for an application made during your first week of C programming. The functions are generally reasonably-sized, the code is comprehensible and the design decisions are understandable given the size and purpose of the application. My remarks mostly center around improving the design to support new features and avoid bugs. The ...


14

Use enums to give names to numbers It would be great if you could write PAWN instead of 1, since it will be much clearer what you are doing in the code. The way to do this is to declare an enum for all possible chess piece types: enum Type { NONE = 0, PAWN, ROOK, BISHOP, KNIGHT, QUEEN, KING, }; I added NONE as well, it will be ...


14

The CompareVersions() function in this answer uses subtraction for comparison. This is considered to be bad practice - it leads to bugs and potential security holes. (Yes, the post does say "if we can ensure that the version values are small enough to avoid integer overflow", but that pretty much requires the caller of this function to know the result ...


14

Here are some things that may help you improve your code. Use all required #includes The code uses memcpy, so it should #include <string.h>. It might still compile on your machine, with your compiler, but it's not portable. Think about potential errors As one of the comments correctly notes, if one of the entries has the value of zero, this line will ...


13

There is a bug in this line: password[i] = symbols[rand() % 26]; symbols is only 21 characters long, so this line triggers undefined behavior when rand() % 26 is greater than 21 (and when rand() % 26 is exactly 21, it puts a null byte in the password). You meant for this 26 to go with letter instead, and for the 21 to go with symbols. This would be less ...


13

Bug: When you feed an empty file to your program, it ends up in an endless loop. Bug: the 24th letter of the English alphabet is X, not S. Instead of const char * you should rather declare const char alphabet[], to make the code match the comment above it. Don't confuse strings and pointers to strings. The authors of the cs50 library do that, and they do ...


13

Make sure Doxygen picks up comments after struct members It's good to add a comment after each member in a struct declaration to describe what that member does. To make Doxygen pick up the member documentation as well, use //!< or ///< instead of // to start the comments with. Use "filename" or "path", not "file" describe filenames You will be ...


13

Return the condition When we test for equality, a == b is either 1 if they're equal or 0 if they're not (see C11 §6.5.8 Relational operators). Therefore we can simply return the expression instead of an if-else construct: bool fifo_is_full(struct fifo * fifo) { return fifo->elements == fifo->size; } bool fifo_is_empty(struct fifo * fifo) { ...


12

Prof's transcribed code has weaknesses #include<stdio.h> int main(){ int n; while(scanf("%d",&n)==0 || n <2 || n >20){ while ( getchar () != ’\n’); printf( "Wrong. Try again\n"); } printf ( "%n\n",n); return 0; } Infinity loop When input is closed after a scanf("%d",&n) == 0, getchar() can eventual ...


12

Watch your memory allocations and deallocations. In both cases, you've got defangIPaddr returning a const char * to heap-allocated memory, which needs to be freed by the caller... but it can't be freed, because free expects a non-const void* as its argument. Functions that return ownership-of-a-heap-allocation to the caller should (A) return char*, not const ...


12

General Observations There is a real lack of communications with the user of the program. There is no error checking performed on the possible input. While the code really doesn't do that much it is too complex and very hard to maintain. Communications With the User Unless the user knows exactly how to call the program, the program will crash with no ...


12

Design Your list includes the head of the list. struct ll_LinkedList { ll_LinkedListNode* head; }; You could make a lot of your code a lot simpler by including two other values (the tail and the length). struct ll_LinkedList { ll_LinkedListNode* head; ll_LinkedListNode* tail; size_t size; }; This would make sure you don't have to ...


12

Redundant code As the 4 functions are nearly identical, consider coding helper functions. At a minimum, I recommend a printf_prefix(const char *ansi_color, const char *pass); To form a helper function of the first part with a va_list parameter is tricky. See below. Time I'd code for a fixed width time // %d:%d:%d %02d:%02d:%02d Flush For the more critical ...


11

I'm not particularly familiar with C, but I have a few observations that may be helpful. I like the attention to zeroing out your digest before you start. Using uninitialised memory is the sort of bug that can go unnoticed for a while, and it's good that you've caught it. At the same time, I'm slightly alarmed that you're doing everything with raw loops, ...


11

Don't strtok + atoi. Use strtol, which (a) doesn't need a mutable input, (b) has much better error handling and reporting, and (c) eliminates the need for independent validation. An example of use would be char * end; pVer->major = strtol(str, &end, 0); if (*end != '.') { // major is not a number. return suitable_failure; } str = end + 1; ...


11

This program reads 4 byte codepoints (in BIG ENDIAN) from a file strictly called "input.data" and creates another file called "ENCODED.data" with the relative encoding in UTF8. Needless to say, that's a weird way of storing code points. I know UTF-16, but UTF-32BE (just the code point in big endian form) is not widely used, although Python seems to use ...


11

My requirement requires me to have indentation. I am unsure how to apply that indentation in my code. Just indent your C code exactly the same way you'd indent Python code. Start at the left margin (column 0), and then each time you "go in a level" (in the body of a function, or an if or while or for, or when breaking an expression across multiple lines), ...


Only top voted, non community-wiki answers of a minimum length are eligible