С++. Решение квадратного уравнения

#cpp #инспекция_кода

Написал программу для вычисления корней квадратного уравнения.
Сам, можно сказать, новичок в программировании, и хотелось бы узнать мнение тех кто
имеет опыт в программировании.
Насколько код ниже соответствует современным требованиям? Какие основные ошибки и
замечания можно сделать к нему?
Огромное спасибо всем! Приму и учту любую критику.

#include 

using namespace std;

double first_koef = 0, sec_koef = 0, third_koef = 0;// коэфициенты квадратного уравнения
double discriminant = 0;
double result_1 = 0, result_2 = 0;
double func_discr(double a, double b, double c); // прототип функции

int main() {
    cout << "Write x1, x2, x3 please" << endl;
    try {//начало try
        if(cin >> first_koef){}//если всё ОК - ничего не делаем
        else {
            throw 1; // выбрасываем исключение      
        }
        if(cin >> sec_koef){}//аналогично
        else {
            throw 1; 
        }
        if(cin >> third_koef){}//аналогично
        else {
            throw 1; 
        }
        func_discr(first_koef, sec_koef, third_koef); // функция подсчёта дискриминанта
        result_1 = (-sec_koef + sqrt(discriminant)) / 2*first_koef;//первый корень
уравнения
        result_2 = (-sec_koef - sqrt(discriminant)) / 2*first_koef;//второй корень
уравнения

        if(result_1 == result_2) {//если результаты одинаковые - выводим один
            cout << "X = " << result_1 << endl;
            _getch();
        }else{//если нет, оба результаты
        cout << "X1 = " << result_1 << "\tX2 = " << result_2 << endl;
        _getch();
        }
    return 0;   
}//end of try
catch(int i) {
    cout << "ERROR in koef! Press any key to exit" << endl;
        _getch();
        return 1;
}//end of catch
}//end of main

//Вычисление дискриминанта.
double func_discr(double a, double b, double c) {
    try {
        discriminant = b*b - 4*a*c;
        if(discriminant < 0) {
            throw 2;
        }else {         
            return discriminant;
        }
    }
    catch(int i) {
    cout << "CRITICAL ERROR! D < 0!" << endl;
    _getch();
    }

}

    

Ответы

Ответ 1

Чуток подправленый вариант, правда в логику не вникал, что там должно получиться.

#include 

using namespace std;

double discr(double a, double b, double c) {
    double d = b * b - 4 * a * c;
    if (d < 0)
        throw std::runtime_error("D < 0!");
    return d;
}

int main() try {
    cout << "Enter a, b, c" << endl;
    cin.exceptions(istream::failbit | istream::badbit);
    double a, b, c;
    cin >> a >> b >> c;

    double d = discr(a, b, c);
    double x1 = (-b + sqrt(d)) / (2 * a);
    double x2 = (-b - sqrt(d)) / (2 * a);

    if (x1 != x2)
        cout << "X1 = " << x1 << "\tX2 = " << x2 << endl;
    else
        cout << "X = " << x1 << endl;

    return 0;
} catch (std::exception& e) {
    cout << e.what() << endl;
    return 1;
}



выброшен ваш странный include за ненадобностью
глобальные переменные заменены на локальные по месту использования
исключения сразу включаются для std::cin, чтобы не обрабатывать отдельно каждый случай
(раз вы уж решили использовать исключения)
исключения обрабатываются в одном месте
добавлены скобки (не знаю почему, просто верю Harry)
переименованы переменные
???????
что-то ещё по мелочи


Если предполагается переиспользование в дальнейшем, код решателя можно вынести в
отдельную функцию:

std::pair  solver(double k1, double k2, double k3) {
    double d = discr(k1, k2, k3);
    double x1 = (-k2 + sqrt(d)) / (2 * k1);
    double x2 = (-k2 - sqrt(d)) / (2 * k1);
    return std::make_pair(x1, x2);
}


Ответ 2

Как минимум, нельзя так злоупотреблять исключениями.

Не нужен полный путь в #include.

Тут

(-sec_koef + sqrt(discriminant)) / 2*first_koef;


Нужны скобки 

(-sec_koef + sqrt(discriminant)) / (2*first_koef);


Вычислять дискриминант через глобальную переменную не нужно. Вообще, чем меньше глобальных
переменных - тем лучше. Вычисление дискриминанта вообще не такое сложное, чтобы его
в отдельную функцию выносить. Но если выносить - то достаточно

double discr(double a, double b, double c)
{
    return b*b - 4*a*c;
}


и уж точно не надо обрабатывать в ней событие, что дискриминант меньше нуля (тем
более что при этом у вас функция возвращает непойми что)...

Вот, для начала...


Ответ 3

Зачем такие страсти с препроцессором

Какой-то страшный инклуд сверху. Выбросить, заменить стандартными.

Пространство имен замусорено. Сохранить (Y/N/MayBe)?

using namespace в общем случае засоряет глобальное пространство имен. Допускаются
частные варианты - using std::cout, как пример.

Долой глобальные сущности, пока они ДЕЙСТВИТЕЛЬНО не востребованы

Глобальные переменные в данном случае бессмысленны. В общем случае, глобальные сущности
вредны для дизайна и от них необходимо избавляться.

Выдача строк строго по талонам после дождичка в четверг у свистящего рака

Определение нескольких переменных на одной строке не соответствует моему личному
code-style. Переменная на строке должна быть ровно одна и к ней должен быть комментарий.

Исключительно глупые исключения

Исключение можно выбрасывать более осмысленное - либо унаследованное от std::logic_error,
либо просто завести свой тип.

Чертовы египетские скобки

 if(cin >> first_koef){}//если всё ОК - ничего не делаем
        else {
            throw 1; // выбрасываем исключение      
        }


Я понимаю, Кернигану и Ритчи строки выдавали по талонам. Но зачем себя мучать?

if(cin >> first_koef)
{
}   //если всё ОК - ничего не делаем
else 
{
   throw 1; // выбрасываем исключение      
}


Блоки кода визуально отделены и читабельность лучше. Не надо выискивать скобки по
всему тексту.

Функция общается с программой через глобальную переменную

Мастдай однозначно. В реальном проекте с автора будет спущена шкура во время code-review.
Спущена с особым цинизмом.

ИТОГИ

Самое страшное тут - глобальные переменные и самоуправство с препроцессором. Ревью
у меня вы не прошли.