Demonstration of ChessBoard Traveling (CoderByte) Problem


  Have the function ChessboardTraveling(str) read str which will be a 
  string consisting of the location of a space on a standard 8x8 chessboard with no pieces on the board along with another space on the
  chessboard.
  
  The structure of str will be the following:
  
  \$(x,y)(a,b)\$ where \$(x, y)\$ represents the position you are
  currently on with \$x\$ and \$y\$ ranging from 1 to 8 and \$(a, b)\$
  represents some other space on the chessboard with \$a\$ and \$b\$
  also ranging from 1 to 8 where \$a &gt; x\$ and \$b &gt; y\$. Your program
  should determine how many ways there are of traveling from \$(x, y)\$
  on the board to \$(a, b)\$ moving only up and to the right. 
  
  Example: if str is \$(1,1)(2, 2)\$ then your program should output 2 because there are only two possible ways to travel from space
  \$(1, 1)\$ on a chessboard to space \$(2, 2)\$ while making only moves
  up and to the right.


Are there any improvements I can make?

#include &lt;algorithm&gt;
#include &lt;functional&gt;
#include &lt;iostream&gt;
#include &lt;iterator&gt;
#include &lt;sstream&gt;
#include &lt;string&gt;
#include &lt;vector&gt;

std::string delete_Punctuation(std::string&amp; str)
{
    std::string noPunctString = "";

    str.insert(str.length()/2," ");

    for (auto character : str)
    {
        if (!ispunct(character))
        {
            noPunctString.push_back(character);
        }
    }

    return str = noPunctString;
}

bool check_If_PointXY_Is_Less_Than_PointAB(std::vector&lt;int&gt; v)
{
     return (v.at(2) &gt; v.at(0) &amp;&amp; v.at(3) &gt; v.at(1));
}

long long int find_Factorial(unsigned int n)
{
    if (n == 0)
    {
       return 1;
    }

    return n * find_Factorial(n - 1);
}

int number_Of_Steps(std::string&amp; str)
{
    str = delete_Punctuation( str );

    std::istringstream iss( str );

    std::vector&lt;int&gt; coll((std::istream_iterator&lt;int&gt;( iss )),std::istream_iterator&lt;int&gt;());

    int steps = 0;

    if (check_If_PointXY_Is_Less_Than_PointAB(coll))
    {
        int xDistance = coll.at(2) - coll.at(0);
        int yDistance = coll.at(3) - coll.at(1);

        int totalDistance = xDistance + yDistance;

        //combination formula
        steps = find_Factorial(totalDistance)/(find_Factorial(xDistance) * (find_Factorial(totalDistance - xDistance)));
    }

    return steps;
}
I see some things that may help you improve your code.

Fix the input parsing

The idea is sound but the implementation is flawed. Instead of the complex method currently used, I'd recommend using this single function:

std::replace_if(str.begin(), str.end(), 
        [](char ch){ return !std::isdigit(ch); }, ' ');


Using this, we're guaranteed that the string will contain only spaces and digits.

Use objects

I'd suggest that one could make good use of a Point2D object to make calculations very clear:

struct Point2D {
    int x = 0;
    int y = 0;
    int dx(const Point2D &amp;other) const {
        return x - other.x;
    }
    int dy(const Point2D &amp;other) const {
        return y - other.y;
    }
    friend std::istream&amp; operator&gt;&gt;(std::istream &amp;in, Point2D &amp;p) {
        return in &gt;&gt; p.x &gt;&gt; p.y;
    }
};


Reduce the numerical range for calculations

The current code uses a generic factorial which boils down to $$\frac{(dx + dy)!}{dx! dy!}$$  The maximum for \$dx\$ or \$dy\$ is 7, so the largest factorial required is \$14! = 87178291200\$.  ... Or is it?  In fact, we could consider instead how to generically calculate $$\frac{n!}{m!}$$ with \$m &lt; n\$.  A simple way to do this is like so: 

constexpr unsigned factDelta(unsigned n, unsigned m) {
    return n &gt; m ? n * factDelta(n-1, m) : 1;
}


And the ordinary factorial function could be expressed as:

constexpr unsigned fact(unsigned n) {
    return factDelta(n, 1);
}


This can reduce the range of the multiplications somewhat, especially if we always choose the larger of \$dx\$ or \$dy\$ to divide.  This means that the return value will always be \$&lt;= 7! = 5040\$ which easily fits into an unsigned.  Note, however, that these functions will overflow if they're given numbers outside that range.  For that reason, I'd recommend making them static functions and/or doing range checking.

Don't alter passed parameters

The current number_Of_Steps() routine takes a reference to a string and then alters the string.  It's not strictly wrong, but it's probably not the best interface design.  I would not expect that the passed string was altered.  Two ways to address this would be to pass a const reference or to simply declare the argument as std::string str which makes a local copy.  Because of the need to alter the string (by removing punctuation), I'd recommend the latter approach.

A worked example

Here's an alternative function that uses the functions and objects described above:

int alt(std::string str) {
    std::replace_if(str.begin(), str.end(), 
            [](char ch){ return !std::isdigit(ch); }, ' ');
    std::istringstream in(str);
    Point2D a; 
    Point2D b;
    in &gt;&gt; a &gt;&gt; b;
    int dx = b.dx(a);
    int dy = b.dy(a);
    if (dx &lt;= 0 || dy &lt;= 0) {
        return 0;
    }
    int m = dx + dy;
    if (dy &gt; dx) {
        std::swap(dx, dy);
    }
    return factDelta(m, dx) / fact(dy);
}