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 ...
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 ...
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 ...
12
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 ...
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 ...
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 ...
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);
...
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. ...
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.
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 ...
8
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&...
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 ...
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 ...
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)...
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 ...
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.
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
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 ...
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 ...
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;
...
...
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 ...
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 ...
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,...
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 ...
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 ...
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 ...
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.
...
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 ...
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>
...
Only top voted, non community-wiki answers of a minimum length are eligible
