Инспекция: нахождение корней квадратного уравнения на C++

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

Помогите, как можно улучшить программу по нахождению корней квадратного уравнения.

#include
#include

using namespace std;

double dis(double a, double b, double c);//Псевдоним функции по нахождению дискриминанта

int main()// main функция
{
   double a, b, c, xa, xb, disb;//переменные, необходимые для программы
   cout << "Введите коэфиециент а: ";// Следующие строчки это сбор данных
   cin >> a;
   cout << a << endl;
   cout << "Введите коэфиециент b: ";
   cin >> b;
   cout << b << endl;
   cout << "Введите свободное слагаемое с: ";
   cin >> c;
   cout << c << endl;
   cout << "КОНЕЧНЫЙ ВИД: " << a << "x^2" << "+(" << b << ")" << "x" << "+(" << c
<< ")" << "=0" << endl;//выводит конечный вид уравнения
   double ld;//переменная, в которую будет записан возвращаемый результат функции dis()
   ld = dis(a, b, c);// запись
   if (ld < 0)//ветвление для защиты от отрицательного дискриминаната
   {
      cout << "Ой, все, решений нет." << endl;
   }
   else
   {
      disb = sqrt(ld); //вычисляем квадратный корень
      xa = ((-b) + disb) / (2 * a);// находим корни
      xb = ((-b) - disb) / (2 * a);
      cout << "ПЕРВЫЙ Х РАВЕН: " << xa << endl;//выводим корни
      cout << "ВТОРОЙ Х РАВЕН: " << xb << endl;
   }
}

double dis(double a, double b, double c)// функця по нахождению дискриминанта
{
   double xld;
   xld = ((b*b) - (4 * a*c));
   cout << "Дискриминант равен: " << xld << endl;
   return xld;
}

    

Ответы

Ответ 1

Программу можно "улучшить" так: решать не так в лоб, как когда-то в школьной тетрадке
писали, а с учетом особенностей поведения компьютерной плавающей арифметики. В частности,
существенно лучшим в этом отношении является следующий подход к решению

double d = b * b - 4 * a * c; // Дискриминант

double q = b >= 0 ? (-b - sqrt(d)) / 2  : (-b + sqrt(d)) / 2; 
// Здесь узнается наше родное `-b +- sqrt(d) / 2 * a`, 
// но пока что без `a` в знаменателе

double x1 = q / a;
double x2 = c / q;


Те, кто знаком с формулами Виета, легко увидят и математическую правильность решения.
А вот почему следует поступать именно так, можно почитать в классической статье (а
на русском языке - у Моулера, Малькольма и того же Форсайта в книге "Машинные методы
математических вычислений")

Вкратце, идея заключается в том, что в плавающей арифметике во избежание потери точности
рекомендуется избегать сложения относительно больших чисел, близких по абсолютному
значению, но имеющих разные знаки - результат может получиться катастрофически неточным.
В "лобовом" решении подвыражение -b - sqrt(d) может страдать от этой проблемы, если
b отрицательно. Вышеприведенный подход при вычислении промежуточной величины q всегда
выполняет сложение чисел с одинаковыми знаками.


Ответ 2

#include
#include


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

using namespace std;


Никогда не пишите using namespace std;, особенно в глобальном пространстве имен.
Это приводит к конфликтам имен, причем иногда ошибки компиляции не будет, но программа
будет вызывать какую-то стандартную функцию вместо Вашей.

double dis(double a, double b, double c);//Псевдоним функции по нахождению дискриминанта


Это называется "декларация" или объявление функции, а не псевдоним. Нет никакого
смысла ее тут использовать. Пишите свои функции в начале, в main() в конце программы.
Не надо экономить на буквах. Переименуйте в discriminant.

int main()// main функция


No shit bro. Не надо писать бессмысленные коментарии, и так понятно что она main
и что она функция.

{
   double a, b, c, xa, xb, disb;//переменные, необходимые для программы


Не надо объявлять несколько несвязанных переменных в одной декларации (ES10). Объявляйте
переменные только в месте их использования (ES21) и всегда их инициализируйте (ES20)
(если есть чем).

   cout << "Введите коэфиециент а: ";// Следующие строчки это сбор данных


Не надо писать комментарии, пишите код. Вынесите это в функцию void collect_input(double&
a, double& b, double& c) или лучше Coefficients read_coefficients(std::istream& stream),
тогда название функции будет говорить о том что тут делается.

   cin >> a;
   cout << a << endl;


Здесь не нужен endl, используйте '\n'.

   cout << "Введите коэфиециент b: ";
   cin >> b;
   cout << b << endl;
   cout << "Введите свободное слагаемое с: ";
   cin >> c;
   cout << c << endl;
   cout << "КОНЕЧНЫЙ ВИД: " << a << "x^2" << "+(" << b << ")" << "x" << "+(" << c
<< ")" << "=0" << endl;//выводит конечный вид уравнения


Не надо писать бессмысленные комментарии, в сообщении уже написано что это.
Сложный код лучше выносить в отдельные функции, например print_formula(a, b, c);

   double ld;//переменная, в которую будет записан возвращаемый результат функции dis()
   ld = dis(a, b, c);// запись


Должно быть double dis = discriminant(a, b, c);. Почему вообще ld? Не используйте
непонятные сокращения. Также лучше использовать вывод типов: auto dis = ...;

   if (ld < 0)//ветвление для защиты от отрицательного дискриминаната
   {
      cout << "Ой, все, решений нет." << endl;


Не надо мешать в одну кучу вычисление корней уранения, обработку случая отсутствия
корней и логирование ошибок.
Вынесите вычисление корней в отдельную функцию, например 
bool try_solve(const Coefficients& a, Roots& x). Альтернативный вариант - std::vector
solve(const std::vector& coeffs).

   }
   else
   {
      disb = sqrt(ld); //вычисляем квадратный корень
      xa = ((-b) + disb) / (2 * a);// находим корни
      xb = ((-b) - disb) / (2 * a);


Должно быть auto xa = ...; или double xa = ...;

      cout << "ПЕРВЫЙ Х РАВЕН: " << xa << endl;//выводим корни
      cout << "ВТОРОЙ Х РАВЕН: " << xb << endl;
   }
}

double dis(double a, double b, double c)// функця по нахождению дискриминанта
{
   double xld;
   xld = ((b*b) - (4 * a*c));
   cout << "Дискриминант равен: " << xld << endl;
   return xld;
}


Не надо засовывать логи в вычисления. Если так хочется залогировать дискриминант,
напишите класс или структуру, которая будет хранить промежуточные результаты вычислений,
и потом их логируйте. Например:

struct QuadraticEquation {
  QuadraticEquation(double a, double b, double c) : a(a), b(b), c(c) {}

  bool solve() {
    discr = calc_discriminant();
    if (discr < 0) return false;
    x1 = ...
    return true;
  }

  double a, b, c;
  double discr;
  double x0, x1;
};


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


Ответ 3

На мой взгляд, имеет смысл:


Вынести функцию решения квадратного уравнения:

int solve(double a, double b, double c, double *x1, double *x2)


a, b, с - коэффициенты
возвращаемое значение - количество корней (или -1 если их бесконечно много)
*x1, *x2 - сами корни (если корней меньше двух, лишние переменные не трогаем)
Сравнивать дробные числа с некоторой точностью EPS, например, 1e-7.
В том числе, сравнить дискриминант с нулём как fabs(d) < EPS.
Убрать функцию вычисления дискриминанта.
Ввод и вывод оставить в main'е.


Ответ 4

Код с минимальными правками (подправил алгоритм):

#include 

using namespace std;

const double EPS = 1E-5;

static bool isEqual(const double& left, const double& right, const double& eps = EPS)
{
   return abs(left - right) < EPS;
}

double dis(double a, double b, double c);//Псевдоним функции по нахождению дискриминанта

int main()// main функция
{
   double a, b, c, xa, xb, disb;//переменные, необходимые для программы
   cout << "Введите коэфиециент а: ";// Следующие строчки это сбор данных
   cin >> a;
   cout << a << endl;
   cout << "Введите коэфиециент b: ";
   cin >> b;
   cout << b << endl;
   cout << "Введите свободное слагаемое с: ";
   cin >> c;
   cout << c << endl;
   cout << "КОНЕЧНЫЙ ВИД: " << a << "x^2" << "+(" << b << ")" << "x" << "+(" << c
<< ")" << "=0" << endl;//выводит конечный вид уравнения
   double ld = dis(a, b, c);//переменная, в которую будет записан возвращаемый результат
функции dis()
   if (ld < 0.0)//ветвление для защиты от отрицательного дискриминаната
   {
      cout << "Ой, все, решений нет." << endl;
   }
   else if (isEqual(0.0, a))
   {
      if (isEqual(0.0, b))
      {
         if (isEqual(0.0, c))
            cout << "Существует бесконечное множество решений." << endl;
         else
            cout << "Ой, все, решений нет." << endl;
      }
      else
         cout << "Х РАВЕН: " << -c/b << endl;
   }
   else
   {
      disb = sqrt(ld); //вычисляем квадратный корень
      if (isEqual(0.0, b))
      {
         cout << "Х РАВЕН: " << disb / (2*a) << endl;
      }
      else
      {
         xa = (-b + disb) / (2*a);// находим корни
         xb = (-b - disb) / (2*a);
         cout << "ПЕРВЫЙ Х РАВЕН: " << xa << endl;//выводим корни
         cout << "ВТОРОЙ Х РАВЕН: " << xb << endl;
      }
   }
}

double dis(double a, double b, double c)// функця по нахождению дискриминанта
{
   double xld;
   xld = b*b - 4*a*c;
   cout << "Дискриминант равен: " << xld << endl;
   return xld;
}


Если вам не принципиально выдавать доп. информацию для пользователя в функции dis,
то сразу напишите там return b*b - 4*a*c; во благо срабатывания RVO, либо вообще откажитесь
от данной функции (ведь вы её используете всего 1 раз), ну, либо можете извратиться
и запехнуть это в макрос, например #define dis(a, b, c) b*b - 4*a*c.
Ну и всю логику решения (именно решения задачи, а не ввода данных), по-моему, неплохо
бы вынести в отдельную функцию хотя бы.



P.S.
По заявкам желающих сделал все как вы любите (аля C-style, кроме потоков ввода/вывода,
конечно), зато сколько внимания уделили такому, на первый взгляд, неприметному вопросу
в сообществе (автору, наверное, приятно). И, да, хорошо потроллили, молодцы :)