patterncsharpMinor
Bishop Diagonal
Viewed 0 times
bishopdiagonalstackoverflow
Problem
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() { { '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 Bish
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() { { '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 Bish
Solution
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:
From this mathematical model and analysis it is very simple to calculate the result.
Some comments on the code:
Because you only use the boolean variable once then just do:
This is rather complicated to understand:
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.
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:
- A chessboard is an 8x8 matrix or coordinate system.
- 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)
- 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(true, "Backwards-Left");Because you only use the boolean variable once then just do:
if (BishopNextDiagonal == bishop2) return new Tuple(true, "Backwards-Left");This is rather complicated to understand:
public string[] Figure(string bishop1, string bishop2, Tuple,
Func> b1, Tuple, Func> b2,
Tuple, Func> operation1, Tuple, Func> 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.
Code Snippets
var boolean = BishopNextDiagonal == bishop2;
if (boolean) return new Tuple<bool, string>(true, "Backwards-Left");if (BishopNextDiagonal == bishop2) return new Tuple<bool, string>(true, "Backwards-Left");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)
{...}Context
StackExchange Code Review Q#160659, answer score: 3
Revisions (0)
No revisions yet.