Podcast #128: We chat with Kent C Dodds about why he loves React and discuss what life was like in the dark days before Git. Listen now.

Hot answers tagged

26

Typo lenght is spelled length. Magic numbers What does 95 signify? You'll want to put this in a named #define or a const. Allocation failure After calling malloc, always check that you've been given a non-null pointer. Allocation failure does happen in real life. Indentation You'll want to run this through an autoformatter, because your if block has ...


24

None of your QUEUE_* functions validate their input arguments before using them. NULL pointers will be a problem, particularly for the pointer-to-pointer arguments. C's memory-allocation functions return a void*, which implicitly converts to any other pointer type. This means that you don't need to typecast the result of calloc. Doing so can actually ...


23

@vnp's code is solid and helpful, but his stringify_state_helper is a single-purpose function, and still leaves a degree of repetition and memory management in stringify_state. I'd rather have general-purpose to_string that takes printf-style arguments, allocates sufficient space for the converted result, and prints into that space, and returns the result: #...


22

Well done. This is not a complete review, but instead a (short) list of possible improvements I found when I skimmed your code. Documentation First of all: thank you! It's great to have documentation. Note that there is some debate whether to put the documentation into the header or the source. I'd like to remark that I would put only a @brief ...


18

First of all: nice work! It's easy to read and understand. Program organization It's very good that you split the task to small functions. Reading the body of main reveals nicely the overall flow. Ideas for further improvement: place does two things: it reads input from user and updates the state of the board. It would be good to separate these logically ...


17

p = malloc(n*sizeof *p); This is dangerous if n gets large, because the multiplication could overflow. After the overflow, too little memory has been allocated but code will continue without detecting the error. This is especially dangerous if n comes from untrusted source, such as some file format or remote user. Then it gives attacker easy way to ...


17

This answer uses pointer-casting for type-punning just to save space. In practice keep using your union (safe in ISO C99, and in C++ as a GNU and MSVC extension) or memcpy (safe in C and C++). This pointer-casting is only safe in MSVC, or GNU-compatible compilers with -fno-strict-aliasing Initial approximation Packed bit fields are not only unnecessary ...


16

I see a number of things that may help you improve your program. Since the existing review covered a lot of good points, this review will cover the parts not already mentioned. Use the correct form for main There are exactly two allowed version of main, according to the standard, and yours isn't one of them. This code has this: int main(int argc, const ...


15

The first thing that will help is to switch this from a recursive algorithm to an iterative one. This will prevent the stack overflow that prevents you from solving 25x25, and will be a bit faster to boot. However to speed this up more, you will probably need to use a smarter algorithm. If you track what numbers are possible in each square, you will find ...


15

Allocations can fail Don't use buffer until we know it's not null. (And no need to multiply by sizeof (char), since that's automatically a no-op). Please remember to free() it too (at least as an option). That reduces false positives with Valgrind's memory checker. I/O operations can fail Always check that your I/O succeeds. Consider this scenario: we ...


15

Following @Martin R's comment, I'll make my comment above a solution: printf already supports what you're trying to achieve with your if-then-else jungle: #include <stdio.h> int main() { int T; scanf("%d", &T); for (int i = 1; i <= T; i++) { int N; scanf("%d", &N); int hours = N / 60; int ...


15

/* Asks the user for string input. * Returns a pointer to the string entered by the user. * The pointer must be freed. */ Slightly misleading in that this function doesn't ask for input. (As written it is not responsible for printing the prompt.) Perhaps also should clarify the intended behavior: If there is input that is not terminated by a newline, ...


14

Yes, there is a cleaner way: if (a.major != b.major) { *result = a.major < b.major; } else if (a.minor != b.minor) { *result = a.minor < b.minor; } else if (a.patch != b.patch) { *result = a.patch < b.patch; } else { *result = a.build < b.build; } return OKAY; I reordered patch to come before build since that's how it is usually ...


14

I am only looking for some feedback on my coding style. Formatting is good. I hope it is auto formatted. Respect the presentation width Rather than oblige a horizontal scroll bar, auto format to a narrower width to avoid that. Avoid dogma "to ONLY USE ONE return statement in functions and NOT to use things like break, continue or go-to." --> This is a ...


13

Avoid numbered variables char space_1 = ' '; char space_2 = ' '; char space_3 = ' '; char space_4 = ' '; char space_5 = ' '; char space_6 = ' '; char space_7 = ' '; char space_8 = ' '; char space_9 = ' '; int space_1y, space_1x; int space_2y, space_2x; int space_3y, space_3x; int space_4y, space_4x; int space_5y, space_5x; int space_6y, space_6x; int ...


13

This looks really nice! Here we go: Is the API well thought and idiomatic? Mostly. For a library as simplistic as this, you probably want to avoid creating a special enum when returning NULL on error will suffice. For this you could make the QUEUE_initialize() function return a pointer to the queue_t instead of having a queue_t** passed as an argument. ...


13

Minor stuff ... Allocate to the object, not type The below is easier to maintain. // char **aux = malloc(sizeof(char *)) char **aux = malloc(sizeof *aux) // aux[0] = malloc(bufsize * sizeof(char)); aux[0] = malloc(sizeof *(aux[0]) * bufsize); Avoid Exploit Below code is undefined behavior is the first character of user input is the null character. It ...


12

Here are a number of things that may help you improve your program. Eliminate global variables where practical Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. For this program, it would be easy and natural to wrap nearly all of the global variables in a ...


12

You test the return value of fwrite, which is good. However, fread may fail as well. Since fread doesn't distinguish error from end of file, you should call ferror: while ((num_elements = fread(....)) > 0) { .... } if (ferror(src)) { handle_error } fprintf(stdout), while technically valid, looks strange. A printf suffices....


12

These declarations are not prototypes: void getDataFromRapidSrcFile(); int getTotalPositions(); These declare functions that can be called with any number of arguments. It appears that they should take no arguments; we indicate that like this: void getDataFromRapidSrcFile(void); int getTotalPositions(void); It's a good idea to make the same change ...


12

Naming It's unconventional to name a type with all-uppercase - we normally reserve those names for preprocessor macros, to warn readers that they need treating with care. Avoid such names for ordinary identifiers. Avoid using identifiers that begin with an underscore - in many situations, those names are reserved for use by the implementation, which could ...


11

Prefer Symbolic Constants Over Magic Numbers There is a header file that should be included, stdlib.h, that provides some standard symbolic constants such as EXIT_SUCCESS and EXIT_FAILURE. It might also be better to define buf_size and cpy_buf_siz as symbolic constants. If you are using a C compiler use #define to define your constants, if you are using the ...


11

Looking at the performance, the two versions should perform just about identically. The second version has one less call/return, which can save a couple of CPU cycles, but if you have it multiple places in your code the additional code bytes and cache misses can overshadow that. Either way you probably won't notice a difference. Looking at readability and ...


11

Consider factoring it out into a function: static size_t stringify_state_helper(State * state, char * buf) { return snprintf(buf, ....); } char * stringify_state(State * state) { size_t len = stringify_state_helper(state, NULL); char * buf = malloc(len + 1); if (buf) { stringify_state_helper(state, buf); } return buf; }


11

Whilst your code works, there are a number of simplifications that you might try. As Reinderien says, get rid of "magic" numbers Having done that, declare a single string containing all 95 characters with the special ones last. This does away with all the strcat code. It's good practice to declare has_special_characters as type bool. You will have to ...


11

Avoid global variables - there's no need for t and v to exist outside main(). Always check the return value of scanf() before using the written values. Don't assume that CHAR_BIT is 8, or that sizeof (short) is 2. Neither of those is portable. Don't assume a particular ordering of bit fields within a struct - that's entirely compiler-dependent. Portable ...


10

Your instruction list: #define READ 10; #define WRITE 11; #define LOAD 20; #define STORE 21; #define ADD 30; #define SUBTRACT 31; #define DIVIDE 32; #define MULTIPLY 33; #define BRANCH 40; #define BRANCHANG 41; #define BRANCHZERO 42; #define HALT 43; has a few issues. First of all, you shouldn't add semicolons after those defines. Also, this would be ...


10

Bug As Carsten points out, you need to allocate \$(\text{length}\cdot 2)+1\$ bytes, rather than \$(\text{length}\cdot2)\$ to account for the null terminator sprintf() adds. Formatting Most C formatting guides do not include spaces around the arguments to function calls, nor the expressions within an if-statement. For an example of a C style most C ...


10

In C, initialization and destruction should be done at the same level of abstraction. This is important because it defines who is responsible for the memory. There are two good ways to follow this guideline: Allocate and deallocate in the API's init/destroy functions (your first code example). fopen does this although it maps files rather than regular ...


10

There's two things I'd change to simplify your code: Use a two dimensional array. Sure you can do the conversion from a 2d position to 1d easily, but the compiler can do and board[1][1] is rather more obviously the middle of the board than board[4]. Instead of hardcoding your logic for what positions you have to check, think about a programmatic approach ...


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