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

Question

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

#include<iostream>
#include<cmath>

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;
}

Answer 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 всегда выполняет сложение чисел с одинаковыми знаками.

Answer 2

#include<iostream>
#include<cmath>

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

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<double> solve(const std::vector<double>& 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;
};

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

Answer 3

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

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

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

   a, b, с - коэффициенты
   возвращаемое значение - количество корней (или -1 если их бесконечно много)
   *x1, *x2 - сами корни (если корней меньше двух, лишние переменные не трогаем)

2. Сравнивать дробные числа с некоторой точностью EPS, например, 1e-7.
   В том числе, сравнить дискриминант с нулём как fabs(d) < EPS.

3. Убрать функцию вычисления дискриминанта.

4. Ввод и вывод оставить в main'е.

Answer 4

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

#include <iostream>

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, кроме потоков ввода/вывода, конечно), зато сколько внимания уделили такому, на первый взгляд, неприметному вопросу в сообществе (автору, наверное, приятно). И, да, хорошо потроллили, молодцы :)