New answers tagged

0

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


4

Your program could be a lot shorter and simpler if you followed the "Unix philosophy." Read from stdin and write to stdout. This eliminates your need to process arguments, and eliminates the for(j) loop. Trust fread and fwrite to do I/O buffering on their own. (They do.) Then your entire program would be something like #include <stdio.h> ...


2

Here is my take on the problem, a function which accepts any string length and only allocates exactly enough memory: //#include <malloc/_malloc.h> #include <stdlib.h> // malloc, free #include <assert.h> // assert #include <stdio.h> // printf, fprintf /** * "Defangs" a string by replacing all `.` with `[.]`. * * @param ...


5

There is quite a lot to remark. In general, I think you spend too much time implementing things by hand, when the functionality is also found in the standard library or other commonly available libraries. Furthermore, your program does not do proper error checking, possibly causing silent data corruption. Here is a list of issues I found: Typedefs When ...


7

Consider using asprintf() Just like you are using strdup() to simplify making a copy of a string, consider using asprintf() to print a string without having to worry about allocating memory yourself. This will greatly simplify your code: char* defangIPaddr(const char* address) { char* defanged; int ip[4]; if (sscanf(address, "%d.%d.%d.%d&...


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


3

Some things to note; When doing time related things in C++, use std::chrono, not clock(). Your log will not work if it's used in multiple files. Instead of being in a .h file, put the definition of the logger in a cpp file and build both together. Use include guards for header files #ifndef LOG_H #define LOG_H // Code #endif Define ...


0

#include <stddef.h> #include <stdlib.h> void *ft_realloc(void *old_ptr, size_t old_size, size_t size) { if (size == 0 && old_ptr != NULL) { free (old_ptr); } else if (old_ptr == NULL) { if (old_ptr = malloc(size)) { ft_memset(old_ptr, 0, size); return (old_ptr); ...


3

Use the standard library This all started when I wanted to make a simple program [...] Your program is much more complicated than necessary because you are not using the standard library. While it may be an interesting challenge to see how to write a program without using the standard library, you already did that. Now it just is not a productive use of ...


3

It would be helpful for you to break up the code into functions, that will simplify each task. Process the command line arguments first, then once you have the delimiter process the text files. Declare the variable delim above any logic and initialize it to the default value Process the command line to get the actual delimiter. Process the input file. This ...


6

Congrats on starting to program in C. I started on my own as well, so I know how overwhelming it can be at first. I'm going to be hitting you with a lot in a couple of seconds, so I want you to know that it's not possible to internalize all of this information in one day, and this is an awesome start. I agree with both of the previous answers, so I'm not ...


5

@G.Sliepen covered most of what I would have. A few spare things: Particularly since this is a one-translation-unit (one-file) program, mark every function except main to be static. Consider not declaring left and right structures at all, reusing pair; or at least typedef aliases to pair rather than re-declaring all of the members Use a separate structure ...


9

Avoid global variables The larger the project, the higher the chance that if you use global variables, that you have conflicting global variable names. Try to avoid them when possible. Here are some generic rules you can follow: Declare variables in the function that first uses them. Pass variables as arguments to other functions that need to access them. ...


6

I suggest reading Donald Knuth's article Structured Programming With goto statements (1974) Originally published in Computing Surveys 6, and the second chapter of his book Literate Programming. There he makes excellent points on both sides of the goto debate.


8

Quuxplusone's answer briefly mentioned making this data-driven, and I've fleshed out the idea here. The key realization is that each room varies based only on a few pieces of information: a numeric label, a description string, a set of valid directions, and the label of the room that each valid direction leads to. The original implementation deals with this ...


6

My previous review focused solely on the use of goto. Since then, various answers about state machines and code repetition have popped up. So here is another answer regarding everything else except goto & suggestions for how you could rewrite this better. C Language Binary literals are not standard C and should therefore be avoided. I can see why you ...


4

May someone teach me some hidden tricks in using GOTO? goto is kind of a one trick pony. But is part of a family of control transfer constructs. All the looping and decision structures are in a sense a refined or specialized application of a goto. So from that pov, using goto is advisable only if it cannot be done with any of the "normal" control ...


10

I'm looking at the goto usage in this program, and I'm going to be controversal here and say it's not that bad. I've seen much worse. Here is a list of things this program does not do. This program never uses goto to go around a variable initialization except where the value is clearly thrown out. This program never does a goto into an inner block. All ...


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


5

I'll take an alternative tack from the other answerers: Your code organization is not bad, and all that remains to do is eliminate redundancy. Notice that every room in your game has the same basic structure: ROOM7: giveline(); printf("\"Give a man a LOOP and you feed him FOR a WHILE;\n"); printf(" teach a man a GOTO and you feed him for ...


4

The program you give as an example would be better-designed as a finite state machine. The states could be implemented as mutually-recursive tail calls, which a modern compiler would optimize to jump instructions, just like a goto. You could then represent the states as function pointers and eliminate the switch. Joshua has a demonstration of code somewhat ...


9

Lack of Error Checking on User Input The function getdir() should check for valid input, perhaps is should receive an array of valid directions. When an invalid direction is entered there should be a message to the user that the input was invalid. DRY Code The use of goto is forcing you to repeat code that shouldn't be repeated such as where(2, DIR_W); ...


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


4

readline buf is a pointer. sizeof(buf) is either 8 or 4, depending on the architecture. bzero(buf,sizeof(buf)); only clears 8 (or 4) bytes of the buffer. Similarly, sizeof(strlen(fd.BUF)) is equal to sizeof(int), and 256 - sizeof(strlen(fd.BUF) always evaluates to 252, regardless of the fd.BUF contents. Doesn't look right, and may lead to a buffer overrun. ...


2

Memory ownership This: char* errmsg = _strdup(messageBuffer); LocalFree(messageBuffer); return errmsg; is problematic. The documentation for _strdup says that The _strdup function calls malloc to allocate storage space for a copy of strSource and then copies strSource to the allocated space. So you're allocating, reallocating, doing one free, and leaving ...


1

If this code goes into production then the asserts for the success of calloc() will be optimized out of the code if the code is optimized. Prefer if statements with calls to fprintf(stderr,. The parameter rate is never used, this can lead to maintenance errors in the future. For maintenance reasons the ability of someone else to understand the code I would ...


3

Just some minor things in your code. Is a bit strange that you have a function call free_url and you dont have one called init_url where you alloc and do the memset. My suggestion is that you have another function for that and you move your memset of the function int parse_url(char *start, struct url *url) { char *end, *delim; memset(url, 0, sizeof(*...


2

One thing that I would do different is create a queue_alloc_item so you can decide to use malloc or another memory manager in the future. You have: int *my_node = malloc(sizeof(int)); for ints which always uses malloc. When there are clients using this already and you find a better way to replace the memory manager for small objects you can't change this ...


2

DRY. queue_push assigns queue_back to the input_node no matter what. Consider if (input_queue->front == NULL) { assert(input_queue->back == NULL); input_queue->front = input_node; } else { assert(input_queue->back != NULL); input_queue->back->next = input_node; } input_queue->back = input_node; (asserts ...


5

For consistency with your use of the <stdbool.h> header and the bool macro, your functions that return a bool should use return true; or return false; instead of returning 0 or 1. There will be no difference in the code but it is easier for a person reading the code to recognize what the return is (and this would also align better with your function ...


2

in function: void init_queue(struct priority_queue **pq, size_t capacity) after the final closing brace '}' there is a semicolon ';'. This results in the compiler outputting: untitled1.c:41:2: warning: ISO C does not allow extra ‘;’ outside of a function [-Wpedantic] There are also some warnings about implicit conversions. When compiling, always enable ...


2

... major concern is, is it more efficient to get the number of scores as a Command Line Argument or is the current approach better in case if sometime in the future someone designs a Interactive Front-End for it? "more efficient" needs to focus on order of complexity, else one wastes valuable coding effort in premature optimizations. OP's code ...


4

Avoid double pointers. They are absolutely unwarranted in all the functions (except init, but see below). Prefer returning a value to a side effect. In the client code, which I presume is along the lines of struct priority_queue * pq; init_queue(&pq, capacity); I have to read the source of init_queue to see that there is a side effect of modifying ...


3

Reduce the number of loops The most time-consuming part of your program is to loop through the array. Notice that you have done this twice, why can't you keep a track of the total sum while taking the input? #include <stdio.h> float average(int sum,int number); int main(void) { int number_of_integers,number,sum = 0; printf("Enter the ...


6

Avoid repetition Whenever you are repeating yourself twice or more often, you should immediately find some way to get rid of the repetition. You probably already know how to do this. For example, in this case, just add more for-loops: one to calculate the sum, and another to print all the elements once you have found a set of consecutive elements that sums ...


4

Don't hide pointers Don't hide pointers in a typedef: typedef struct moduleIF_CTX *moduleIF_CTX_t; This makes it really hard to spot when things are passed by value or by pointer in the rest of the code. You could make it more explicit: typedef struct moduleIF_CTX *moduleIF_CTX_ptr_t; But I would just do this: typedef struct moduleIF_CTX moduleIF_CTX; Yes,...


6

Don't use system() for trivial tasks Calling system() means starting a new shell process, which in turn has to parse the command and execute it. The clear command is not a built-in for Bash, so the shell in turn will start a new process to execute /usr/bin/clear. And it's just a program, it's not magic; clear itself is also written in C. And all it ...


6

General Observations Welcome to code review, nice first question. I would leave the licensing information out since stack exchange uses the Creative Commons Attribution-ShareAlike license. The comment block at the top is rather helpful otherwise. FYI, this compiles fine on Windows 10 in Visual Studio 2019 Professional, but doesn't link (usleep() is undefined)...


4

Indent your code! Most of your code is completely unindented, which makes it very hard to read. While the compiler can easily scan your code and count { and } signs to tell where each function and code block begins and ends, for humans this is much more difficult and prone to mistakes. That's why it's a good idea to indent the lines inside each function and ...


5

Repeating short comings The indentation of your post is very bad (and others) of previous post with scant improvement here. Omitting #1 productivity step Turn on all warnings. Below code suffers from mis-match of specifier and type. A good well enabled compile will warn. My compiler provided about 10 warnings. Example: long long complete_integer = 0; ... ...


6

There is no indentation except in the function revers_digits(). The indentation within that function is fine, you should correct the rest of the program because it is basically unreadable. While this may compile in a free online compiler, the code is broken for 2 reasons. The first is that in a one file program such as this one for the C programming language ...


3

Some remarks with respect to the get_base_name() function: size_t vs int: You correctly start with size_t arg_len = strlen(arg); but then pass arg_len to get_last_backslash_index() which takes an int argument. Depending on the strictness of your compiler this can cause a “Implicit conversion loses integer precision” warning. I suggest to use size_t ...


1

Don't read a file character by character Reading a file a character at a time is slow. Even if standard input is buffered, then you still have the overhead of individual calls to fgetc(), which has to check each time if there is at least one character in the input buffer, and if so remove it from the buffer and return it, otherwise read some more data in the ...


3

Only #include what you need, where you need it In graph.h, you #include <stdlib.h> and <string.h>, but you don't use anything from those headers inside graph.h itself, so you should not #include anything there. Instead, in graph.c, you need to #include <stdlib.h> in order to use malloc() and free(), but you don't need anything else. Avoid ...


2

Overview I don' think the way you are using poll() is affective. You are basically moving the busy loop from send()/recv() to the poll() function but then giving up when there is a timeout. If your socket is on loopback that may work great but anything coming across the internet is going to potentially have long waits at some point thus causing your reads to ...


1

How can I simplify my solution by reducing too much line of codes in an efficient way? Simplify and drop value. Not needed and as used may overflow. Perhaps use a wider type to reverse int values like 1000000009. long long complete_integer = 0; while(number) { complete_integer = complete_integer*10 + number%10; number=number/10; }


3

How can I simplify my solution? Is there any better strategy should I follow to solve these types of problems? When using an int only solution, a common problem occurs with values like 1000000009, that when reversed, 9000000001, exceed the int range. To solve the range issue, only form the reverse of the first half of the digits and compare that to the ...


8

You say "Any number of digits are allowed" but then try to read into an int, which can hold only very few digits. So at that point you already lost. Also, even if the entered number fits into an int, the reverse might not: Enter an integer number: 1000000003 Completed integer number : -1294967295 Better use a string instead of an int.


10

Indentation The indentation of your post is very bad, mainly because it is very inconsistent. It doesn't really matter what indentation style you choose, as long as you consistently follow the style. However I strongly recommend the following things: Use spaces around binary operators, after keywords, after comma's. Use a single empty line around functions ...


2

Your function is mallocing a value, and then overwriting it with a pointer to a local array, and then return the pointer to local array, which doesn't exist after the end of the function. This may work in certain cases, not work in others, and crash in others. This line doesn't copy the array, it changes the pointer. You probably meant for this line: ...


Top 50 recent answers are included