Hey everyone, first post here, fairly new to programming and I wanted a bit of feedback on the little calculator I wrote, all code is here, feel free to take a look, maybe checkout a branch, clone it, whatever. All feedback is appreciated, no matter how positive or negative it might be, thanks!
Edited by Peliex
on
Hi, and welcome.
Here are a few notes:
- You don't need the <windows.h> header. You can replace it with <stdlib.h> required for system( "pause" ). But you can also replace system( "pause" ) with a getchar( ) or scanf( ) (no need for stdlib). Since it's a simple program using those would make the program compile on other operating system without any change.
- scanf( "%s", input ); would probably fail if the user inputs more character then your buffer - 1. I believe you can do scanf( "%99s", input ); to limit the number of character to 99 but I'm not sure. Another way would be to use a loop to only read one character at a time with getchar and exit the loop if you reach the maximum or the user pressed enter.
- Instead of comparing with 48, 57 ... you can use the character: '0' '9' '*' '/' ... (whith the quotes) to keep your program readable.
Here are a few notes:
- You don't need the <windows.h> header. You can replace it with <stdlib.h> required for system( "pause" ). But you can also replace system( "pause" ) with a getchar( ) or scanf( ) (no need for stdlib). Since it's a simple program using those would make the program compile on other operating system without any change.
- scanf( "%s", input ); would probably fail if the user inputs more character then your buffer - 1. I believe you can do scanf( "%99s", input ); to limit the number of character to 99 but I'm not sure. Another way would be to use a loop to only read one character at a time with getchar and exit the loop if you reach the maximum or the user pressed enter.
- Instead of comparing with 48, 57 ... you can use the character: '0' '9' '*' '/' ... (whith the quotes) to keep your program readable.
Hi Peliex,
The use of system("PAUSE") is common in windows's console applications and some books suggest to use it, but it is non-portable and useless. For example, if you use Visual Studio to write the code, you can simply go to the project setting > Linker > System and set Subsystem to console. Then, if you run the application with Ctrl+F5, you will see that visual studio automatically add the system("PAUSE") at the end of the application. Any other good IDE provide similar solutions (ofter simpler solutions).
Anyway, it is a console application, so you probably want to launch it directly from a console and, in that case, the console will remain open when the application finish and you will hate that system("PAUSE") at the end of the application.
Note that the use of scanfs for string is considered unsafe. Replace it with fgets() (with stdin as the file stream).
From an usability point of view, I suggest to move the formula as an argument of the application. For example, if you want to calculate 1 + 2 you do something like this (in the console)
even if some character can be problematic, I need to think about it.
You can also consider to ask for a particular notation. Since you are new in software development and you probably haven't studied Mathematical logic, you probably do not know that the prefix and suffix notations do not need parentheses. The prefix notation -- lisp use it even if it likes parentheses a lot -- is like this
which is equivalent to
. The suffix notation is the opposite, that is
. This notation is less legible than the other two but it makes possible to implement the calculator very efficiently (it use less memory of the others). It works in this way:
The use of system("PAUSE") is common in windows's console applications and some books suggest to use it, but it is non-portable and useless. For example, if you use Visual Studio to write the code, you can simply go to the project setting > Linker > System and set Subsystem to console. Then, if you run the application with Ctrl+F5, you will see that visual studio automatically add the system("PAUSE") at the end of the application. Any other good IDE provide similar solutions (ofter simpler solutions).
Anyway, it is a console application, so you probably want to launch it directly from a console and, in that case, the console will remain open when the application finish and you will hate that system("PAUSE") at the end of the application.
Note that the use of scanfs for string is considered unsafe. Replace it with fgets() (with stdin as the file stream).
From an usability point of view, I suggest to move the formula as an argument of the application. For example, if you want to calculate 1 + 2 you do something like this (in the console)
1 2 | [xyz] calculate "1 + 2" Result is 3 |
You can also consider to ask for a particular notation. Since you are new in software development and you probably haven't studied Mathematical logic, you probably do not know that the prefix and suffix notations do not need parentheses. The prefix notation -- lisp use it even if it likes parentheses a lot -- is like this
1 | + 3 / - 7 3 2 |
1 | (3 + ( (7 - 3) / 2 ) |
1 | 3 7 3 - 2 / + |
1 2 3 4 5 6 7 8 9 10 | stack new elements inserted [] <- 3 [3] <- 7 [3, 7] <- 3 [3, 7, 3] <- - [3, 4] <- 2 [3, 4, 2] <- / [3, 2] <- + [5] <- end of string [] -> 5 |
Thanks for your insight, I've been looking for the right terms to search a better method of implementing the calculations, but, as you'll notice, the file is built entirely from the command line, so I cannot refrain from including the system("pause") in my program as the "cl" command doesn't do it for me. I'll definitely take your idea of using the other string input versus scanf. I hadn't heard it was unsafe before. Also, mrmixer, I thank you for your insight on the stdlib include instead of windows.h, since it saves me some compile time, ideally, I can get this whole thing working inside a GUI soon.
I found a method of making the program use system("PAUSE") selectively, whenever the program is run as a standalone .exe, it will pause so that you can read the outputs. But, whenever it is run from the command line, it no longer pauses.
I used wincon.h and user32.lib to do it. It uses the commands getConsoleWindow and getWindowThreadProcessId to find the value of the command line, which is later compared to the cmd process id, and if they're the same, it will pause the program, otherwise it just continues as normal. Thanks for your insight though, because that's what helped me get to this, and I'm going to change the scanf function to fgets soon.
I used wincon.h and user32.lib to do it. It uses the commands getConsoleWindow and getWindowThreadProcessId to find the value of the command line, which is later compared to the cmd process id, and if they're the same, it will pause the program, otherwise it just continues as normal. Thanks for your insight though, because that's what helped me get to this, and I'm going to change the scanf function to fgets soon.