6
\$\begingroup\$

I have solved this kata on Codewars.com and it passes all tests. I am looking for refactoring tips and proper coding tips as well. Also I am a beginner that is trying to Improve. I would like to shorten my code and clean it up.

Task

In the Land Of Chess, bishops don't really like each other. In fact, when two bishops happen to stand on the same diagonal, they immediately rush towards the opposite ends of that same diagonal.

Given the initial positions (in chess notation) of two bishops, bishop1 and bishop2, calculate their future positions. Keep in mind that bishops won't move unless they see each other along the same diagonal. Example

For bishop1 = "d7" and bishop2 = "f5", the output should be ["c8", "h3"].

For bishop1 = "d8" and bishop2 = "b5", the output should be ["b5", "d8"].

The bishops don't belong to the same diagonal, so they don't move.

Input/Output

[input] string bishop1

Coordinates of the first bishop in chess notation. [input] string bishop2

Coordinates of the second bishop in the same notation. [output] a string array

Coordinates of the bishops in lexicographical order after they check the diagonals they stand on.

My solution that passes all tests on Codewars.com and that I want refactored is below

    class Kata
{
    public  bool ChessBoardCellColor(string cell1, string cell2)
    {
        string x;
        string y;
        var xaxis = new Dictionary<char, int>() { { 'a', 1 }, { 'b', 2 }, { 'c', 3 }, { 'd', 4 }, { 'e', 5 }, { 'f', 6 }, { 'g', 7 }, { 'h', 8 } };

        var input1 = cell1.ToCharArray();
        int j;
        xaxis.TryGetValue(input1[0], out j);

        x = ((j % 2 == 1 && input1[1] % 2 == 1) || (input1[1] % 2 == 0 && j % 2 == 0)) ? "black" : "white";

        var input2 = cell2.ToCharArray();
        int k;
        xaxis.TryGetValue(input2[0], out k);

        y = ((k % 2 == 1 && input2[1] % 2 == 1) || (input2[1] % 2 == 0 && k % 2 == 0)) ? "black" : "white";

        return x == y ? true : false;
    }



    public   Tuple<bool, string> BishopDia(string bishop1, string bishop2)
    {
        if (ChessBoardCellColor(bishop1, bishop2))
        {
            int add = 1;
            string BishopNextDiagonal;
            string BishopPreviousDiagonal;
            string BishopNextLeftDiagonal;
            string BishopPreviousRightDiagonal;
            while (add <= 8)
            {
                var x = Convert.ToChar(bishop1[0] + add);
                var value = Char.GetNumericValue(bishop1[1]) + add;
                BishopNextDiagonal = x + "" + value;

                var boolean = BishopNextDiagonal == bishop2;

                if (boolean) return new Tuple<bool, string>(true, "Backwards-Left");

                var x1 = Convert.ToChar(bishop1[0] - add);
                var value1 = Char.GetNumericValue(bishop1[1]) - add;
                BishopPreviousDiagonal = x1 + "" + value1;

                var boolean1 = BishopPreviousDiagonal == bishop2;

                if (boolean1) return new Tuple<bool, string> (true, "Forwards-Right");

                var x2 = Convert.ToChar(bishop1[0] - add);
                var value2 = Char.GetNumericValue(bishop1[1]) + add;
                BishopNextLeftDiagonal = x2 + "" + value2;

                var boolean2 = BishopNextLeftDiagonal == bishop2;

                if (boolean2) return new Tuple<bool, string>(true, "Backwards-Right");

                var x3 = Convert.ToChar(bishop1[0] + add);
                var value3 = Char.GetNumericValue(bishop1[1]) - add;

                BishopPreviousRightDiagonal = x3 + "" + value3;

                var boolean3 = BishopPreviousRightDiagonal == bishop2;

                if (boolean3) return new Tuple<bool, string>(true, "Forwards-Left");

                add++;
            }

            return new Tuple<bool, string> (false, "");
        }
        else return new Tuple<bool, string> (false, "");
    }

    public string[] Figure(string bishop1, string bishop2, Tuple<Func<int, bool>,
        Func<int, bool>> b1, Tuple< Func<int, bool> , Func<int, bool>> b2, 
        Tuple<Func<int, int>, Func<int, int>> operation1, Tuple<Func<int, int>,Func<int,int>> operation2)
    {

        var count = 1;

        var bishopcoordinateX = ' ';
        var bishopcoordinateX1 = ' ';

        while (count < 8)
        {
            count++;
            var value = Char.GetNumericValue(bishop2[1]);
            if (b2.Item1((bishop2[0] - 0)) && b2.Item2((int)value))
            {
                bishopcoordinateX = Convert.ToChar(operation2.Item1(bishop2[0]));
                value = operation2.Item2((int)value);
                bishop2 = bishopcoordinateX + "" + value;
            }

            var value1 = Char.GetNumericValue(bishop1[1]);
            if (b1.Item1((bishop1[0] - 0)) && b1.Item2((int)value1))
            {
                bishopcoordinateX1 = Convert.ToChar(operation1.Item1(bishop1[0]));
                value1 = operation1.Item2((int)value1);
                bishop1 = bishopcoordinateX1 + "" + value1;
            }
        }

        return new string[] { bishop1, bishop2 }.OrderBy(c => c).ToArray();
    }



    public string[] BishopDiagonal(string bishop1, string bishop2)
    {
        var x = BishopDia(bishop1, bishop2);
        if (x.Item1)
        {

            int Max = 'h' - 0;
            int Min = 'a' - 0;

            if (x.Item2 == "Backwards-Left")
            {
                    return Figure(bishop1, bishop2, 
                    new Tuple<Func<int, bool>, Func<int, bool>>(z => z > Min, z => z > 1 ), 
                    new Tuple<Func<int, bool>, Func<int,  bool>>(z => z < Max, z => z < 8 ),
                    new Tuple<Func<int, int>, Func<int, int>>(z => z - 1, z => z - 1 ),
                    new Tuple<Func<int, int>, Func<int, int>>(z => z + 1, z => z + 1 ));
            }
            else if(x.Item2 == "Forwards-Right")
            {

                  return Figure(bishop1, bishop2,
                  new Tuple<Func<int, bool>, Func<int, bool>>(z => z < Max, z => z < 8),
                  new Tuple<Func<int, bool>, Func<int, bool>>(z => z > Min, z => z > 1),
                  new Tuple<Func<int, int>, Func<int, int>>(z => z + 1, z => z + 1),
                  new Tuple<Func<int, int>, Func<int, int>>(z => z - 1, z => z - 1));

            }
            else if(x.Item2 == "Backwards-Right")
            {
                 return Figure(bishop1, bishop2,
                 new Tuple<Func<int, bool>, Func<int, bool>>(z => z < Max, z => z > 1),
                 new Tuple<Func<int, bool>, Func<int, bool>>(z => z > Min, z => z < 8),
                 new Tuple<Func<int, int>, Func<int, int>>(z => z + 1, z => z - 1),
                 new Tuple<Func<int, int>, Func<int, int>>(z => z - 1, z => z + 1));
            }
            else if (x.Item2 == "Forwards-Left")
            {
                return Figure(bishop1, bishop2,
                new Tuple<Func<int, bool>, Func<int, bool>>(z => z > Min, z => z < 8),
                new Tuple<Func<int, bool>, Func<int, bool>>(z => z < Max, z => z > 1),
                new Tuple<Func<int, int>, Func<int, int>>(z => z - 1, z => z + 1),
                new Tuple<Func<int, int>, Func<int, int>>(z => z + 1, z => z - 1));
            }
            else return new string[] { bishop1, bishop2 }.OrderBy(c => c).ToArray();
        } 
        else return new string[] { bishop1, bishop2 }.OrderBy(c => c).ToArray();
    }
}
\$\endgroup\$
2
\$\begingroup\$

First of all I'll upvote the question for the effort.

You split the problem into subproblems and you show good understanding of C# idiomatic as well as general programming skills.

On the other hand I think you overcomplicate the solution, because your general analysis of the problem is a little too "chessish". Instead I would try with some kind of "mathematical" model:

  1. A chessboard is an 8x8 matrix or coordinate system.
  2. A chess field can be converted into a matrix coordinate set (x, y) by: field = "f4" => (x, y) = (field[0] - 'a', field[1] - '1') = (5, 3) (zero based)
  3. Two fields are on the same "diagonal" if their offset has equal length in x and y: abs(field1.X - field2.X) == abs(field1.Y - field2.Y)

From this mathematical model and analysis it is very simple to calculate the result.

Some comments on the code:

 var boolean = BishopNextDiagonal == bishop2;
 if (boolean) return new Tuple<bool, string>(true, "Backwards-Left");

Because you only use the boolean variable once then just do:

if (BishopNextDiagonal == bishop2) return new Tuple<bool, string>(true, "Backwards-Left");

This is rather complicated to understand:

    public string[] Figure(string bishop1, string bishop2, Tuple<Func<int, bool>,
        Func<int, bool>> b1, Tuple<Func<int, bool>, Func<int, bool>> b2,
        Tuple<Func<int, int>, Func<int, int>> operation1, Tuple<Func<int, int>, Func<int, int>> operation2)
{...}

What are b1, b2, operation1/2 doing? A better naming would be suitable.

Tuples are nice objects for holding (temporary) data, but I wouldn't use them in the above situation. Instead I would make a class or find another approach.

\$\endgroup\$
  • \$\begingroup\$ I was storing an operation of addition or subtraction in my func's so then I could just have one method not many methods. \$\endgroup\$ – Gringo Jaimes Apr 14 '17 at 16:54
  • \$\begingroup\$ Wow thank you for your critiques, I am going to make those changes! Maybe better said a new version of my program Thank you very much again. \$\endgroup\$ – Gringo Jaimes Apr 14 '17 at 17:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.