Hot answers tagged

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

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

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


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


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

Only include the header files that you need Looking at tape.h, it only contains declarations and no definitions. As such, the header files only serve to bloat up the source code and increase compilation time. You should move them to tape.c. Use static methods if possible If I look at main.c, the only functions that are utilized are initializeTape, interpret ...


10

Welcome to CodeReview! The program doesn't look too bad, however there are some ways to improve it: Naming things The biggest issue with your code is that there is a bunch of single-letter variable names (i...k), some magic constants (2, 3, 4, 17 and 99), and nested while-loops. If I wasn't told what this program would do and didn't run it for myself, I ...


10

I will offer some suggestions. Please note I have no experience with Mac OS, but I believe most of it will still apply. Using system is not particularly bad in your case since you use static arguments in those calls excluding the possibility of command injection. However, since your first C program is almost completely made up of system calls, you should ...


9

rand_ranged_inclusive(a, b) is a surprisingly long function name, for how little information it communicates to the reader. I have frequently seen versions of this function named randint(a, b) or randint0(b) — but where the possible outputs are between 0 inclusive and b exclusive! Using "half-open ranges" like this has huge advantages in C, because ...


8

You could use perror() to give more useful information from library-call failures. For example, consider if (file == NULL) { fprintf(stderr, "Can not open file %s\n", argv[1]); return EXIT_FAILURE; } We could get better error message (e.g. "file not found", "permission denied", etc) like this: if (!file) { perror(...


8

long both; Using a long for storing a cast pointer can be both wasteful and too little. Just use the dedicated uintptr_t. Even if your implementation might not be fully up to C99 (MS greets), it probably has <stdint.h> and the typedef. pStNode newNode = (StNode *) malloc(sizeof(StNode)); The above line demonstrates three bad ideas: More pieces to ...


7

As mentioned by a commenter on the question, neither version satisfies the requirement to return a new array. I'll leave that for you to fix yourself (demonstrating your understanding of memory allocation to your interviewer). The division version requires some modification to work when one or more inputs are zero. I suggest keeping track of the position ...


7

This function can be written like this: char* readStdinLine() { char* buffer = NULL; size_t bufsize = 0; ssize_t characters = getline(&buffer, &bufsize, stdin); if (characters == -1) { free(buffer); buffer = NULL; } else if (buffer[characters-1] == '\n') { buffer[...


7

Array length Explicitly declaring NUM_JOBTYPES 4 and also having alg_names be automatically sized is risky. There are two options, either of which would bring you more consistency: Simply declare alg_names[NUM_JOBTYPES] Define NUM_JOBTYPES to be equal to the size of alg_names via sizeof Otherwise, a sleep-deprived future you (for example) could adjust one ...


7

long isn't necessarily a good choice of integer type for storing pointers - luckily <stdint.h> provides us with uintptr_t which is guaranteed to be wide enough for this purpose (always prefer an unsigned type when working with bitwise operations). I dislike hiding pointer types behind typedefs such as pStNode. I think it's clearer to use the pointer ...


7

For a beginner the code is neat. Most is already given by the previous, much more elaborate answer. One thing though w.r.t. coding: C is unique in that it can use aliases, pointers to a variable/field. For instance below current first is an alias of head and after that of a node's next. With *current = ... you can fill the original variable. void ll_insert(...


6

Overall Impression At first glance everything looks great, the indentation is good, the use of enums over Magic numbers is good, the only obvious thing is the comments between functions that look like /*-------------------------------------------------------------*/, they are not useful. First looks can be deceiving. Avoid Global Variables It is very ...


6

If you are going to write raw terminal codes to output, I'd advise encapsulating that into a well-named function. Few of us can tell by looking exactly what effect \e[1;1H\e[2J\e[?25l will have. There's a lot of use of while loops that, while not functionally incorrect, would be more idiomatically written as for loops, enabling other developers to read and ...


6

Process a stream of data instead of a single string Your program works by passing the input as a command-line argument, but what if you want to encode or decode a whole file? What if the input is huge? I would try to modify the program so that it can run from a file, or from standard input if no file is specified, so you can do for example: ./vigenere key &...


6

Premature optimization I just noticed that I can do an XOR-swap instead of a temp variable. This would also allow the SWAP_INT macro to remain untyped That's unfortunately incorrect - xor-swapping in fact has a reliance on type. Aside from your declared int type, the "naive" implementation of your swap would work with (for example) struct ...


5

You could eliminate the special case here: //left most element special handling new_arr[0]=1; //swipe up for(int i=1; i<arr_size; i++) { mult_prefix *= nums[i-1]; new_arr[i] = mult_prefix; } by assigning before multiplying, and bringing index 0 into the loop: //swipe up for(int i=0; i<arr_size; i++) { new_arr[i] = mult_prefix; ...


5

Proper scoping int mult=1; //product of prefix or suffix elements mult above is only used in the first loop. The one used in the second loop is defined there, and limited to that loop. This one should be the same. Proper naming j in the second loop is an unexpected name. As it is an outer loop, you should use i (again). int size = sizeof(nums)/sizeof(...


5

Use const when able Functions like ll_at() and ll_length() could code with const to better convey code's usage and allow some select usages and optimizations. // void* ll_at(ll_LinkedList* ll, size_t pos); void* ll_at(const ll_LinkedList* ll, size_t pos); // size_t ll_length(ll_LinkedList* ll); size_t ll_length(const ll_LinkedList* ll); Code looks nice, ...


5

The terminology is a bit sloppy - I would call the operation of going from digits back to alphabetic characters decoding rather than encoding - and our function isn't really either of those: it's counting. (I'm guessing that you're not a native English speaker, so don't take this criticism too harshly!). The interface is surprising: accepting an integer ...


5

Yes, there is a plenty of redundancy here. Let's start with if (time_prefix) part. Consider if (time_prefix) { printf("%d:%d:%d ", tm.tm_hour, tm.tm_min, tm.tm_sec); } printf("\033[0m\033[1;34m[INFO]\033[0m %s", buf); Next step is to unify INFO, WARNING, ERROR, SUCCESS. All of them are identical, except the coloured ...


5

Aside from anything else, like how you might want to save your output as a string and print that, the usleep() function is deprecated and has been removed from version 4 of the Single Unix Specification. Either use nanosleep(), or #define _XOPEN_SOURCE and _POSIX_C_SOURCE to values that will select a version of the system libraries that still supports usleep(...


5

Bug: no null character printf("%s\n", result); fails as result does not point to a string. That data lacks a null character and is short, by 1 of allocated memory needed. // char *output = malloc(output_len); char *output = malloc(output_len +1); output[output_len] = '\0'; Similar problem with decryptor. No protection against non-A-Z characters I'...


5

Your code is not correct. It can crash. First, look at the line fgets(buffer, BUFFERSIZE, stdin); If an error occurs while reading, the state of buffer is indeterminate. You should not continue processing it in that case. You need to detect this by the appropriate means and signal a failure. Second, if the malloc() fails, what will be the effect of the next ...


4

Parenthesized macros The parens around (200) aren't strictly necessary. Most of the time that we see these, it's because you want to enforce order of operations at the calling site, but there are no operations here. Statics This is a one-file (one-translation-unit) program, so all of your methods except main should be marked static. Const It's important ...


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