New answers tagged

0

It has nothing to do with multiple conditions. It is about the logic. If you are interested in it, you can find the answer here.


1

This was... rough. Your method is incorrect @aghast's method is incorrect Your reference outputs are incorrect You're missing unit tests; if you had included some, this would have been caught. First, all three methods along with the unit testing code: #include <assert.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #...


3

First, the bug Consider this: what happens if you have a string that is shorter than maxlen as your input? Assume that maxlen=78, and the input string s is "foo bar". You have strlen(s) == 7. So then pos = 0 at first matches (pos < maxlen), so the while loop starts, the for loop runs, and s[pos + i] is s[0 + maxlen] is s[0 + 78] which is ...


2

Not much to add to the good code and the good review of @G. Sliepen No error checking of pthread_mutex...() calls I'd expect return values checked. Consider an apply() function Consider a function that applies a function to each queue element. Maybe ending early if an error detected. bool queue_apply(queue_t * queue, int (*apply)(void *state, void *ellement)...


2

Your code looks quite nice. I especially like that you have a very consistent API with clear names that follow the conventions of the C++ STL, and the pointer to the queue always as the first parameter. That said, there are some possible improvements. Naming things I would use queue_init() and queue_exit() here. Alternatively, use queue_create() and ...


1

Good formatting Include.h? A companion Generic.h with the public functions definitions is a common and good exercise. Consider a function to test empty-ness bool GenericEmpty(const struct GenericNode *list); Consider a function to peek at the top data void *GenericPeek(const struct GenericNode *list); Consider a pop that also frees void *GenericPopData(...


0

Since this is a single-translation-unit program, mark your panic and help as static. About this: (whoami = strrchr(progName, '/')) ? ++whoami : (whoami = progName); "Yikes". Generously we call this write-only code. Unroll it to char *whoami = strrchr(progName, '/'); if (whoami) whoami++; else whoami = progName; In this loop, your i ...


1

Should I use const in this function argument? Yes. Better conveys to a user code's functionality. Consider the user may only see the declaration void kcr_scene_update(const struct KCR_Scene* scene); (in some header file) and not the definition. Allows increased applicability. Function can be called with const struct KCR_Scene* some_pointer. Without a ...


7

Bug Code attempts to form a string. Yet it lacks a terminating null character and space for it. // static char binary[10]; static char binary[11]; As a static, it is initialized to all zeros. With increased size, an explicit setting of the null character is not needed. OP's code undefined behavior perhaps "works" as a zero may exist just past ...


10

More specific types are available for an eight bit number, such as uint8_t static keyword is dangerous (when used as an output buffer) and results in weird behaviour if your method is called twice The arithmetic can be replaced with a more simple & (bitwise AND) void to_binary(uint8_t x, char *output) { *output++ = '0'; *output++ = 'b'; for (...


5

Only problem is the goto. Can it be avoided without using a function? Candidate goto replacement for (col = bq[row];;) { //NEXT_col: // Delete if (++col == N) bq[row--] = -1; else { for (rd = 1; rd <= row; rd++) if ((old = bq[row-rd]) == col || old == col+rd || old == col-rd) ...


3

void pr_solution(int *bq, int col) { void init_board(int bq[], int v) { These could be declared with static linkage. int main() { Prefer int main(void) to make this declaration a prototype (takes no arguments, rather than unspecified arguments). int row, col, rd, old; All of these could have smaller scope. E.g. for (int row = 0; row >= 0;) { It's ...


1

What does line_size represent? String length (like strlen()), string size or what? With input "abc\n", *line_buffer is "abc" and line_size is 4. With input "abc" End-of-file, *line_buffer is "abc" and line_size is 3. Input error - non handling When fgets() returns NULL due to an input error, consider also returning 0 ...


2

#define ERRALLOC(ptr) \ if (ptr == NULL) { \ printf("Unable to allocate memory for %s", #ptr); \ exit(1); \ } That's a poor way to handle allocation failure, and makes your function pretty much unusable in ...


2

Make more use of calloc() I would use calloc() almost everywhere. It guarantees the memory is zeroed, so you won't have surprises if you forgot to initialize a member variable. I wouldn't worry about performance much; the compiler should be able to optimize the zeroing away if it sees you are setting a member variable to something else. So, use it in ...


2

Overall, it's a lot of effort (perhaps not in the right place) for one line. Textual data typically has lines that are very short, on the order of 80 bytes. If you're so deeply concerned about performance, you need to back out of this function and look at the I/O pattern of the calling program for more context. You'll very likely find that it's more ...


0

A simplification would be the following. int checksum(long card) { int checksum = 0; for (int i = 0; i < get_card_length(card); i++) { int digit = get_nth_digit(card, i); if (i % 2 == 0) { checksum += digit; } else { int num = digit * 2; if (num > 9) // 10, ...


2

This code needs compiling with more warnings enabled: gcc -std=c17 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wstrict-prototypes -Wconversion 254841.c -o 254841 254841.c: In function ‘main’: 254841.c:14:13: warning: conversion from ‘int’ to ‘float’ may change value [-Wconversion] money = dollars_to_cents(...


5

The code can be structured better, so the code is more self-documenting. Self-documenting code doesn't need as many comments, which makes it easier on everyone. Comments that aren't there can't be obsolete either. Take input (height) Output pyramid Those are the only lines I want to see in main. The rest should be in functions. You started out nicely by ...


-2

printf is overkill for printing a single character - you could use putc instead. However, printf provides some useful facilities that we can use to our advantage. Look at this for inspiration: #include <stdio.h> int main(void) { const int width = 8; for (int i = 0; i < width; ++i) { printf("%*s%.*s\n", width-i, "&...


1

Your pointer usage is fine, but there are some other oddities in the code. Floats get assigned to doubles for no discernable reason. There exists an entire loop to make an array positive, which can be accomplished using abs(). There exist multiple return statements which can be reduced to a single expression. A more notable issue is tht the absolute ...


1

BUG: q_init() doesn't return a value. Your compiler should have caught this; perhaps you're running without enough diagnostics turned on? (I think this requires a diagnostic, so perhaps you need a better compiler). BUG: We forgot to copy the terminating null character! for (int i = 0; i < numtest; i++) { q_push(nameq, test[i], strlen(test[i])); } ...


3

Here are some ideas that may help you improve your code. Understand the purpose of CMake Many people misunderstand CMake as a "build system" but it is not. It is a tool for creating a build system. Generally speaking, this means that you describe the desired artifacts (executables to be compiled, tests to be compiled and run, documentation to be ...


5

Here are some things that may help you improve your code. Choose better variable names I understand that N and Q are used within the problem description, but c and d are not and are very short and non-descriptive names. Add comments Comments do not add to compile time and help you (and others!) keep track of what is happening in the code. Add error checking ...


5

Code #pragma GCC optimize("O2") Normally we just pass that as a compiler flag, so we don't get "unrecognised pragma" warnings from other compilers. long int c,d,j,N,Q; These are poor names - they tell us very little about what they are used for. The number of array elements and the number of queries can't be negative. I suggest ...


0

Naming Please don't use ALL_CAPS identifiers for things that are not preprocessor macros: extern bool CMD_LINE; static const char *VAL_CHRS = "+-!^*/%.()\n1234567890'", *OPERS = "+-!^*/%", *DBLS = "+-!", // Can be double *UNRY = "+-!", // Can be unary ...


3

"Utility" libraries It's time to take the training wheels off, so to speak. You need to stop using cs50.h and replace it with standard calls to the C libraries. The implementation for get_string has a careful, dynamically-allocated buffer algorithm that is really not necessary for most purposes and can be replaced with simpler calls that use a ...


1

Whenever I see (e.g.): int foo0, foo1, foo2, foo3, ..., fooN; I think that can be replaced more cleanly with an array. So, I'd rather have: int foo[N]; Thus, I'd change: int32_t micInside0[SIGNAL_LENGTH]; (et. al.) into: int32_t micInside[4][SIGNAL_LENGTH]; That sort of thing occurs in several more places for different variables as well. A good style ...


2

In addition to the other reviews, some misc comments: The switch RUN_REAL_PROGRAM is weird, doesn't seem like something that should be part of the final program - remove it. In general, avoid non-standard C if there is no good reason for it. That is, replace for example non-standard #pragma once with standard C #ifndef SOMETHING_H #define SOMETHING_H ... #...


2

Running strlen() at the beginning is unecessary and can be replaced with a simpler while loop. // A,B are strings unsigned int count_edit_dist(const char *A, const char *B) { unsigned count=0; // a,b are pointers to current letters of A,B const char *a = A; const char *b = B; // while we have characters from either while (*a||*b) { ...


3

Minimum edit distance count_edit_dist() does return a value, yet it is certainly not the minimum possible edit distance as defined. To compute the minimum, a fair amount more code is needed. Small stuff unsigned vs. size_t vs. int For practical uses, makes no difference, yet code would avoid casting by using size_t and making the printf() format strings ...


1

Just a review of a small portion of code: PSet *pset = malloc(sizeof(PSet)); pset->data = malloc (sizeof(char *) * (1 << sset->size)); pset->data[pset->size] = calloc(2, 1); pset->size is not yet assigned a value. pset->data[pset->size] is a Bug. @1201ProgramAlarm sizeof(char *) --> Is that the correct size? I need to look ...


2

Avoid FP functions for integer problems. sqrt(n) problems include potential inexact roots for perfect squares (it is a FP function, not an integer one) and rounding of the double argument when n > 254 or so. Sample alternative; int_sqrt in C Other: Only small stuff: Minor: OK for a narrower type //uint64_t m = n % 10; unsigned m = n % 10u; if (m == 1 || ...


2

Your m digit checking can be simplified and your do while loop can be replaced with the simpler for loop. The constants also seem excessive given that non-zero truthiness is baked into C. int isPrimeNumber(uint64_t n) { if (n==2 || n==3) return 1; if (n%2==0) return 0; // Start near line x=y. uint64_t x = (n / 2) + 2; uint64_t y = n -...


3

malloc vs. realloc Rather than allocate, copy, append ... ... consider re-allocate, append. // char* tempFunctionName = (char*)malloc((functionNameLength+2)*sizeof(char)); // strcpy(tempFunctionName, functionName); // tempFunctionName[functionNameLength] = c; // tempFunctionName[functionNameLength+1] = 0; // free(functionName); // functionName = ...


2

DRY The block while (!ended && c != EOF) { if (c == '(') { ...


3

Overall this isn't a bad effort. I can't see any problems, aside from clobbering the EOF, mostly it's stylistic choices. So good job 👍. Comments go above It's much more common for the comment in a header to be ABOVE the function, rather than below it. This is ok for now as there's only one function, but when you have more it will confuse people. If you're ...


4

Overall, a good and thoughtful effort. Bug: Treats character as EOF When EOF == -1 and code reads a character with the value of 255, the c = getc(fp); likely converts that 255 to -1. Later, the loop incorrectly exits. signed char c; c = getc(fp); if (c == '\n' || c == EOF) return NULL; // Maybe true for wrong reason Instead use an int c to well ...


5

Excellent comment in the header. It's very important to be clear who owns the returned memory and how to release it. It ought to mention that both newline and \0 are considered as end-of-line characters. Good realloc() usage - we don't leak when allocation fails. Some small improvements: #define EXPAND_RATIO 2 Why do we need a preprocessor macro for this?...


0

One declaration per line, please. char File[SavesLen+Len[FirstLen > SecondLen]+2]; is extremely hard to read. In fact, I suspect that there is a bug. Correct me if I am wrong, an intention is to allocate enough memory to accomodate the longest pathname. Now, if FirstLen is indeed greater than SecondLen, the condition evaluates to true, which is in this ...


1

is using NULL in a non-char array a common way to show that the end has been reached? NULL, the null pointer constant, is often used to indicate the end of an array of pointers as done nicely below. It is not a question if the array if "non-char" or not. It is a question if the array elements are pointers or not. Car* my_cars[] = {&chevy, &...


3

In the line printf("\n<Car: %s, Price: $%d>", car->name, car->price); the variable car->price is of type unsigned int, but you are using %d to print it, which is intended for signed int. This causes undefined behavior according to §7.21.6.1 ¶9 of the ISO C standard. However, on most platforms, this will probably just cause the number ...


4

While your code "works" for you, there are a lot of small mistakes in it. I'm listing them straight from top to bottom. #include "stdio.h" Since stdio.h is a header from the standard library, as opposed to a header you define yourself in your project, you should include it using #include <stdio.h> instead. typedef struct Car { ...


6

Make use of the standard library You are reinventing lots of wheels in your programs. There is already a lot that the standard library you are using will do for you. Even in C on Linux, you can use POSIX functions for managing hash tables: hcreate() and hsearch(). But even better is probably to write both the Linux and Windows versions in C++, and make use ...


3

You are relying on the compiler zeroing local variables. For portability initialize them as needed. Use the address of either head or next fields (llnode**). Then deletion will become trivial. Free the deleted node. head node deletion does not printf. As printf was probably just test code, not so important. Two loops (instead of ifs) are more readable; first ...


3

In the function del_element_from_last, when removing the first element of the list, you update llist->head. However, when removing the last element of the list, you do not update llist->tail. If keeping track of the tail of the list is only intended for building the list, and is not intended to be updated by the function del_element_from_last, then you ...


1

General review about pow.h and pow.c. When overflow does not occur, pow_umax() can only return UINTMAX_MAX with pow_umax(UINTMAX_MAX, 1). There is no other base, expo such that pow_umax(base, expo) returns UINTMAX_MAX, regardless of the bit width of uintmax_t due to Mihailescu's Theorem. [Just found that out today.] So setting errno = ERANGE is only ...


Top 50 recent answers are included