Rate the quality of the code (purity, rationality of writing)

Question

Recently began to learn C ++ in other programming languages, there was no previous experience. I do not want to become a "govnokoderom", etc. I'd like to immediately develop a good style of code. Please rate the "Calculator".
Maybe something should be removed, add something. I apologize right away if I have done something “barbarously” by no means I still know, but I want to practice, so I am writing something simple.

#include "pch.h" // Необходимая библиотека (нужна для Visual Studio) #include <iostream> // Необходимая библиотека (Основная библиотека в C++) #include <string> // Необходимая библиотека (нужна для использования "string") using namespace std; float arr[1000000]; // Объвление массива переменных int deistv[1000000]; // Объвление массива действий int main() { setlocale(0, ""); string action; // Введенная информация (либо число, либо действие) int x = 0; // Переменная для работы цикла ввода, увеличивается при введении числа int xt = 0; // Переменная для работы цикла ввода, увеличивается при введении действия int n = 0; // Переменная для работы цикла вывода (используется для создания следующего значения) start: // Ссылка для возврата к вводу данных cin >> action; // Запрос данных if (action == "=") { // При введении "=" отправляет в функцию вывода goto next; } else { if (action == "*") { // Умножение deistv[xt] = 1; // Действие = "умножение" xt++; // Порядок действий goto start; // Возврат к введению данных } if (action == "/") { deistv[xt] = 2; // Действие = "деление" xt++; // Порядок действий goto start; // Возврат к введению данных } if (action == "+") { deistv[xt] = 3; // Действие = "сложение" xt++; // Порядок действий goto start; // Возврат к введению данных } if (action == "-") { deistv[xt] = 4; // Действие = "вычитание" xt++; // Порядок действий goto start; // Возврат к введению данных } else { arr[x] = stoi(action); // Присваиваем данные в массив переменных x++; // Порядок чисел goto start; // Возврат к введению данных } } next: // Функция вывода for (int y = 0; y <= x; y++) { // Тут используем "x" для выделения границы цикла, цикл выполняет действия пока не закончатся ЧИСЛА n = y + 1; // Следующее число if (deistv[y] == 1) { // Умножение cout << arr[y] << " * " << arr[n] << " = "; // Выведение на экран "a * b = " arr[n] = arr[y] * arr[n]; // Число умножается на следующее за ним и результат присваивается второму множителю cout << arr[n] << endl; // Окончательное заполнение формулы на экране "a * b = с" } if (deistv[y] == 2) { cout << arr[y] << " / " << arr[n] << " = "; // Выведение на экран "a / b = " arr[n] = arr[y] / arr[n]; // Число делится на следующее за ним и результат присваивается делителю cout << arr[n] << endl; // Окончательное заполнение формулы на экране "a / b = с" } if (deistv[y] == 3) { cout << arr[y] << " + " << arr[n] << " = "; // Выведение на экран "a + b = " arr[n] = arr[y] + arr[n]; // Число складывается со следующим за ним и результат присваивается второму слагаемому cout << arr[n] << endl; // Окончательное заполнение формулы на экране "a + b = с" } if (deistv[y] == 4) { cout << arr[y] << " - " << arr[n] << " = "; // Выведение на экран "a - b = " arr[n] = arr[y] - arr[n]; // Из числа вычитается следующее за ним и результат присваивается вычитаемому cout << arr[n] << endl; // Окончательное заполнение формулы на экране "a - b = с" } } }

To "=" - input to the console.

First of all, it is necessary to check the correctness of the data, and whether there is any input data at all .
If you close the input stream during the input process, your program will simply loop

Accepted Answer · 2018-11-17T22:35:05

The code looks like intentionally created for use in interviews a tangle of "bad coding" practices.

Global variables
Bare arrays with dimensions in the style of "this is certainly enough."
A strange way to declare local variables at the beginning of a function, and even with meaningless initializers ("initializers for initializers"). (Why is suddenly y correctly declared locally, and n , which is used even more locally than y , has suddenly flown to the beginning of the function?)
Use float instead of double . By the way, if you already used the floating type, then why input is limited only to integers?
Inappropriate goto . And in the second half of the program, a for loop was suddenly used. Why isn't there a goto ?
Magic constants ( 1 , 2 , 3 , 4 ).
Abstract or confusing variable names. (The data array is simply called arr ? Is it surprising that the second array is called deistv , not arr2 . arr2 x and y indices? Are the coordinates on the plane, or what?)
Incomprehensible comments ( xt++; // Порядок действий ?)
Wild way to call header files "libraries".
#include "pch.h" // Необходимая библиотека (нужна для Visual Studio) . No, this is not a "necessary library", this is a consequence of why it is not clear why the mode of precompiled headers is enabled. In the project from one file you do not need precompiled headers.
using namespace std; - in a single implementation file, you can probably ... But still.
Multiple branching implemented through the if ladder makes sense to combine into a single whole through else if , and not write out as a sequence of independent if -s that do unnecessary checks. This is not only a question of optimization, but also a question of the correctness of the code. You have in the first part of the code just contained a potential error caused by this problem, from which only goto start; saves you goto start; .

The variable x shows the number of values in the arr array. Why then the calculation loop iterates from y = 0 to y <= x . There is nothing on the index x . At the same time, the same index is used to access the deistv array. And who said that there is anything in the array deistv ?

10. - pch.h is not a "precompiled header mode", it is just a kind of header file.
The mode of precompiled headers is included in the project settings, and it’s not a fact that it is enabled here.
According to the list of header files it is impossible to understand.
You seem confused by the coincidence of this name with the .pch extension of service files VS.
In clause 10 "it is not clear why the precompiled header mode is enabled by you."
without it, VS gives an error and refuses to include the program .... As for the first points, to be honest I still don’t understand how to declare an array without size in the global.
In principle, the program was originally conceived as a set of functions that would be called in turn.
@GTmix Precompiled headers are disabled somewhere in the project settings.
At the expense of variable length arrays, it is best to use std::vector (or pointers with new[] ) for this.
Even if you really have them included (which is far from a fact), for one source there is no difference whether standard headers are read as part of precompilation or immediately for compilation.
And if you still want to turn off: right-click on the project / "Properties ..." / "C ++" / "Precompiled Headers" / set the very first item in "Not Using Precompiled Headers".
@AnT, well, actually it is needed, and errors will really come up without it.
There are also windows.h , standard library headers, and so on.
True, this file is stdafx.h called stdafx.h in Windows, and I only saw pch.h in a few samples of the Platform SDK.
It's not a matter of precompilation, it’s just convenient to have a single file with headers, common project #define , global constants, etc., etc.
The idea of precompilation arose only later - since we have a common title for each source code, let's compile it once.

Answer 2 · 2018-11-17T20:59:01

In modern programming it is not customary to write a comment to each line. Usually they try to give speaking names to variables, methods, functions, and such a source is read as a completely meaningful text in English.

The comment in the code is an exceptional thing. Your comments in the spirit of "this multiplication" show that you do not know the language in which you are writing - but this is not a reason to litter the listing with these comments, you just need to know the programming language in which you are writing.

Further. You do not structure the program into separate functions. You have one long footcloth of the code for as many as 70 lines: by the end of the method, it’s impossible to remember what was at the beginning. Procedural programming is primarily a skill of decomposing / splitting a large program into a series of simpler ones. They need to be given speaking names. This you can not see in the program.

Further. They told you about goto, but they didn’t say the main thing. Any program with goto can be rewritten to not use goto. In your case, an infinite while (true) loop with a specified exit condition is appropriate.

Further. You do not use such an operator as switch - and write everything through if - but you could write shorter and clearer.

Thank you very much, as I have already said, “In principle, the program was originally conceived as a set of functions that would be called in turn. But I am“ in a rush of writing "completely about them" do 5 minutes ago.
And so I try to write down some thoughts before leaving the program in a comment at the beginning of the file.
And switch - I fell in love with it initially, just here I didn’t have to switch to it with if, the idea of the "output function" was in switch.
@GTmix If you have problems with "forgotten" - you are highly recommended such things as reviewing the code with a live person who will look and remind.
The alternative is to show code on stackoverflow under the heading "code inspection".
For example, you can create a new question and specify a link to this to show the rewritten program to criticize again.
Or, in the most extreme case, write yourself a “checklist: a list of questions of the kind that you need to check after writing the program”
@AK, for the time being, an account has been created exclusively for this purpose, I will throw off my best options for evaluation, I hope they will objectively criticize.

AK ♦ 18.8k eight thirty 77 · Answer 3 · 2018-11-17T19:13:41

The main library must first be written so priority is observed
No comments for goto next
Arrays of small size - what if you need more?

Well, if it is fluent, you can still find IMHO

3) Is it possible to somehow declare dimensionless (dynamic) arrays not in "main ()", when I tried different options, all errors were issued, just a dimensionless array outside functions did not work (

Answer 4 · 2018-11-18T08:30:23

You have a completely digestible C-ishny code. All comments can be made, but they only change the awl on the soap or indicate possible errors. In practice, I have seen the code much worse, but that's not the point. The main thing is that this code is not C ++. If you are learning C ++, then you must have such a word in the program as class . In this sense, you should always consider any task through the lens of OOP. Specifically, in this case, you had to define objects, their hierarchy and interrelation. For example, a number object that falls into an expression object that is added to a calculator object. I do not insist that the architecture should be just that, you can think over your architecture. In this case, you can use STL. In general, I would recommend that you consider this task in such a context and rewrite it using the full power of C ++, STL and OOP.

Shl

Minus. Arrogantly digging into the novice code is petty, and it is necessary to send in the right direction.

"send in the right direction" - "If you are learning C ++, then you must have such a word in the program as class" Well, I don’t know.
Well, is it also necessary to write hello world with classes?
@HolyBlackCat It is directly related to C ++, which a person is studying.
And, in this sense, even the Hello world is not just possible, but must be derived through the class.
Thank you very much for the feedback, I will try to take everything into account, until I just started reading literature on the language, so with the PLO everything is bad, I did not even think about using structuring in a calculator)

Rate the quality of the code (purity, rationality of writing)

4 answers 4

More articles: