C ++ комбинированная функция всегда приводит к 0

Question

Может кто-нибудь сказать мне, почему моя комбинационная функция всегда дает 0?Я также пытался заставить его вычислить комбинацию без использования функции перестановки, но факториал все равно результат 0;

#include <iostream>
#include <cmath>

using namespace std;

int factorial(int& n)
{
  if (n <= 1)
  {
     return 1;
  }
  else 
  {
    n = n-1;
    return (n+1) * factorial(n);
  }
}
int permutation(int& a, int& b)
{
  int x = a-b;
  return factorial(a) / factorial(x);
}
int Combination(int& a, int& b)
{
   return permutation(a,b) / factorial(b);
}

int main()
{
   int f, s;
   cin >> f >> s;

   cout << permutation(f,s) << endl;
   cout << Combination(f,s);


  return 0;
 }

Answer 1

Ваша непосредственная проблема заключается в том, что вы передаете изменяемую ссылку в вашу функцию.Это означает, что здесь у вас есть неопределенное поведение:

return (n+1) * factorial(n);
//      ^^^             ^^^

, потому что factorial(n) изменяет n и имеет неопределенную последовательность (n+1).Аналогичная проблема существует в Combination(), где b дважды изменяется в одном и том же выражении:

return permutation(a,b) / factorial(b);
//                  ^^^            ^^^

Вы получите правильные результаты, если передадите n, a и b по значению , например:

int factorial(int n)

Теперь factorial() получает свою собственную копию из n и не влияет на n+1, который выумножив его на.

---

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

• Избегайте using namespace std; - в нем есть ловушки для неосторожных (и даже для осторожных!).

• Вы можете написать factorial()без изменения n после передачи по значению (а не по ссылке):

  int factorial(const int n)
  {
      if (n <= 1) {
          return 1;
      } else {
          return n * factorial(n-1);
      }
  }
  

• Рассмотрите возможность использования итеративного кода для вычисления факториала.

• Вероятно, нам следует использовать unsigned int, поскольку операции для отрицательных чисел бессмысленны.Вы можете рассмотреть unsigned long или unsigned long long для большего диапазона.

• Вычисление одного факториала и деление на другое не только неэффективно, но и рискует ненужное переполнение (когда a равновсего 13, с 32-битным int).Вместо этого мы можем умножить только на другое число:

  unsigned int permutation(const unsigned int a, const unsigned int b)
  {
      if (a < b) return 0;
  
      unsigned int permutations = 1;
      for (unsigned int i = a;  i > a-b;  --i) {
          permutations *= i;
      }
  
      return permutations;
  }
  

  Это работает с гораздо большим a, когда b мало.

• Мызаголовок <cmath> ни для чего не нужен.

---

Рекомендуемый фиксированный код:

unsigned int factorial(const unsigned int n)
{
    unsigned int result = 1;

    for (unsigned int i = 2;  i <= n;  ++i) {
        result *= i;
    }

    return result;
}

unsigned int permutation(const unsigned int a, const unsigned int b)
{
    if (a < b) return 0;

    unsigned int result = 1;
    for (unsigned int i = a;  i > a-b;  --i) {
        result *= i;
    }

    return result;
}

unsigned int combination(const unsigned int a, const unsigned int b)
{
    // C(a, b) == C(a, a - b), but it's faster to compute with small b
    if (b > a - b) {
        return combination(a, a - b);
    }

    return permutation(a,b) / factorial(b);
}

Answer 2

Вы не рассчитываете значение указателя, которое вы рассчитываете с помощью адреса указателя.