New answers tagged

3

Welcome to Code Review! I'm always glad to see someone who reads the classics. You made some interesting observations about the program. The statement Use more "self-explaining" variable names is absolutely correct, the variable name nb could definitely be improved. I would like to point out that self-documenting might be better than self-explaining. If ...


2

Welcome to Code Review, nice first question. Don't Panic, and always take a towel with you. General Observations This may be a copy and paste error, but the level of indentation is inconsistent. This makes writing, reading and debugging the program very difficult. There are too many comments. Code should be self documenting for the most part, only things ...


3

Firstly, I must apologies that my German understanding isn't fluent enough to read all your comments. I think your intuition is right that you ought to be using functions rather than preprocessor macros - the latter are just too fragile, and anyway, good compilers are able to inline functions. I would recommend -O3 -march=native in your GCC flags if speed ...


1

Should I do away with the #defines and use functions instead? Probably. Speed is everything. No functions. Just #defines. Are you so sure? Have you tested the speed of your application when using regular functions? Optimizing C compilers are pretty good. They'll be able to inline away the functions that you write. Defaulting to writing #define macros ...


3

The usage example is unrealistic, as it fails to show the error handling that's necessary when initialising the stack. Wrapping the initialisation in command-like macros makes it harder to write the correct checks (as we can't just use a return value, we have to inspect the macro to see where the result went): STACK_INIT(8); if (!stack_global) { /* not ...


1

if((ret = malloc(sizeof(*ret)))) { ret->beg = malloc(size); ret->end = ret->cur = ret->beg + size; } You don't check the second malloc's returned value. When out of memory, you're likely to return an object in invalid state (or maybe not, since ret->end and ret->cur initialization is an UB anyway).


5

Overall the code is well-written, consistent and easy to follow. Program design / big picture Having one .c file per function is quite extreme - this just creates a lot of fuss when linking and maintaining the code. This isn't a whole lot of code, so it is hard to justify splitting it in so many different .c files when it could have been placed in a single ...


3

The "terminating" allocators work well for small programs like this; in larger projects or libraries, we want to do something better than terminate the program when allocation fails. A common naming scheme (perhaps taken from Perl) is malloc_or_die() - that's slightly clearer about the behaviour. It's usual to end your error message (and indeed program ...


1

Possibly unneeded variable I don't think this variable is needed: static size_t const front = 0; Just using the constant 0 may or may not be clearer. String concatenation A nice trick that I like to use is string concatenation; instead of using #define ERR(msg) fprintf(stderr, "%s\n", msg) you can do: #define ERR(msg) fputs(msg "\n", stderr) which ...


4

Algorithm As @MinMax pointed out there are limitations to Queue implementations using arrays. I would personally use a linked list to implement a queue rather than an array. The only size limit on the queue implemented with a linked list is how much memory can be allocated. The front of a queue implemented using a linked list can be deleted. There doesn't ...


2

Early returns are OK. if (args < 4) { printf(....); return; } .... emphasizes where the business logic is. The condition (c = **++argv) != '\n' looks sort of strange. It is indeed possible to embed a newline in an argument, but it doesn't warrant a special case. It is just one way to malform an argument, and there are plenty of them. c = d; does ...


5

The delete function is not right. It is very non-optimal to shift every item by one index after each deletion. Imagine the queue has 1 million items and the deletion of front element is needed. From Wikipedia: Fixed length arrays are limited in capacity, but it is not true that items need to be copied towards the head of the queue. The simple trick ...


1

Avoid trying to combine assignment and compare in the same statement. The cast within the statement makes it even hard to read. Also, handle EOF with getchar. You can get EOF with redirected input. Instead of this: while((ch = (char) getchar()) != '\n') { . . . } This: int value = getchar(); ch = (char)value; while (value != EOF and ch != '\n') { ...


0

Realistically, you would want to check that the input characters are acceptable to your program.


1

One issue that I see is that you will not be able to call functions in this library from C++. To be able to do so, you need to wrap stack.h in an extern "C" block: #ifdef __cplusplus extern "C" { #endif /* functions */ #ifdef __cplusplus } #endif


5

Modularization Generally I like well modularized C programs and libraries, however, in this case the library would benefit from having all the functions in one file. This would allow all the function access to certain functions that the user should not see. The functions that the user should not see are: stack_resize(struct stack *, size_t) stack_diagnose(...


14

/* 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, ...


4

1) Would it have been better to let the called function handle the printing of error messages, exiting the program, etc, instead of the main function? It is always better to separate error handling from the rest of the code. But you could have placed the error handling in a separate function. Related to this, you could have created a more meaningful loop ...


3

It seems fairly sane; I only picked up a couple of things: Don't ignore errno You do check for failure, which is good; but you ignore errno. You should use it, and/or perror, to get more detailed failure information. Assign-in-condition There's not a compelling reason to do this: char *pNewline; if ((pNewline = strchr(user_input, '\n')) == NULL) Just ...


3

Overall the code is fairly easy to read and I don't spot any portability issues. Program design The module handling the allocation should also clean up after itself. If transferring this to a proper multi-file program, you would have a lib with a header/code pair like base64.h + base64.c. If there exists a function in base64.c that calls malloc, there ...


1

Suggest using uint64_t (from the header file: stdint.h) rather than long long


4

A few points about improving code It seems that your naming convention for constants is to use a capitalized identifier. If so, that should be applied consistently for all constants including inputFailure. The loop in the function getTermCount can be simplified. A for-loop is clearer IMO. The output of getNthFibonacciTerm is actually off by one term (...


5

This is in addition to @TobySpeight review. Standard Symbolic Constants Since stdlib.h is included, you have access to EXIT_SUCCESS and EXIT_FAILURE which are standard macros implemented on all systems. These work in C++ as well as C. These are more portable than return 0 or return 1. Most modern C compilers will append return 0; so it really isn't ...


7

Don't cast the return value from malloc and family - provided there's a prototype in scope (which there is here, due to our #include <stdlib.h>), the void* result will convert to any pointer type. Always check whether the returned pointer is a null pointer before using it: wsbuffer = malloc(buffer_size * sizeof *wsbuffer); if (!buffer) { fputs("...


2

Portability To be clear, strtoi() and strtou() that OP's code relies on is not in the standard C library. OP's code is limited to the requisites. strtol() may be more complex, yet it is portable throughout all compliant C implementations. Bug - failure to clear errno When strtod() succeeds, it does not change errno, so the tests on errno are testing the ...


2

Precision 1 <= M <= N <= 1,000,000,000 implies a need for 30 bit of precision. (log21,000,000,000 --> 29.897...) float usually has 24 bits of precision (23 explicitly encoded, 1 implied). Code as below can readily lose precision when converting from int to float for values of N > 224. float halfPrice = N; float sum = halfPrice; double ...


3

Your approach is working too hard. The inner loop does K iterations (with could be as large as 1,000,000,000), and most of them are redundant: as soon as halfPrice goes beyond M it will stay beyond M for the rest of the loop. Notice that in the worst case (N = 1,000,000,000, and M = 1) you'll need just 30 iteration to reach M. So, something along the lines ...


3

Formatting This may be a copy and paste error, but generally int main() will start in the first column like the #include does. Other the rest of the indentation needs to be based on that. This was correctly done in your earlier question. #include <stdio.h> int main() { int T; scanf("%d", &T); ... } Functions You may not have run ...


10

If your code is checked by an automated process, you will probably fail all test cases because your code prints a #, which was not asked for. You should better remove it. Just print "Case %d" instead of "Case #%d".


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 ...


1

Kudos on using modern OpenGL for this! We see a lot of OpenGL code around here relying on deprecated functionality, so this is a breath of fresh air. Naming I think your naming needs some work. The name convert_pixels_to_render_coordinates is highly confusing. When I think of pixels, I generally think of an array of RGB(A) values (or possibly in some other ...


2

typedef is your friend You're using C99, so typedef struct { ... } rectangle; instead of the older style. Additional structs If you want to tighten up some of your code, one potential way is to make a coordinate struct. This would halve the number of members of rectangle, for instance. Such nesting would not have a performance impact. It would also be ...


3

I would use uint64_t even if the API asks for an unsigned long. It's far more precise about what it is, and you can always ensure that both are the same. Assuming your C version is >= GNU C11 you can use the following code just below your #includes (actually anywhere, but I like it on top): _Static_assert(__builtin_types_compatible_p(uint64_t, unsigned ...


3

The main advantage of strtol and family is that they compute (for free!) the point where the conversion ended. It is a very valuable information, because usually after extracting the number you want to continue parsing. Your wrappers throw it away. DRY. The switches (which compute result) for integral types is identical. Factor it out into a function.


3

If you need a type with exactly 64 bits, use uint64_t (from <stdint.h>). That will give a clear compilation error if no such type is available. Given that the function never returns anything other than zero, we can move the cleanup of n, n0 and n_max outside the loop, and simply break to reach them. Is indefinite looping really a good output if a ...


5

I don't like this: do { printf("Card number: "); scanf("%lld", &no); } while(no<10); That blank line makes it look like the while (); is a separate (possibly infinite) loop. I recommend writing the while keyword straight after the closing brace, like this: do { printf("Card number: "); } while (scanf("%lld", &no) != 1); (I've ...


3

d_16 = no%10; d_15 = ((no%100)/10)*2; d_14 = (no%1000)/100; d_13 = ((no%10000)/1000)*2; d_12 = (no%100000)/10000; d_11 = ((no%1000000)/100000)*2; d_10 = (no%10000000)/1000000; d_9 = ((no%100000000)/10000000)*2; d_8 = (no%1000000000)/100000000; d_7 = ((no%10000000000)/1000000000)*2; d_6 = (no%100000000000)/10000000000; d_5 = ((no%1000000000000)/100000000000)*...


2

Given that (from your comments) there is one and only one input which would cause overflow, I propose the following check at the beginning of the function: int check_convergence(uint128_t n) { const uint128_t n0 = n; int e; if (n == UINT128_MAX) return 1; do { ... } while (true); } ...


3

Danger! while (isdigit(str[ESC_CHAR_POS + num_digits])) { Promoting char to int (as argument to isidigt()) can sign-extend it. We have to launder the argument through unsigned char on the way: while (isdigit((unsigned char)(str[ESC_CHAR_POS + num_digits]))) { We can make this a bit more readable with a function: static bool ...


5

if (mem_low >= mem_hi || mem_hi < mem_low) The second part of that if is completely redundant.


6

Pointer-arithmetic on void* is an error in standard c. Yes, gcc/clang have an extension assuming that sizeof(void) == 1. Ramp up your warning-level and specify the standard. That's an interesting method to swap two blocks of memory. Using a variable length array invites undefined behavior though, as the amount of stack requested is pretty much unbounded. ...


Top 50 recent answers are included