Problem
I have solved this problem but I am looking for a better way to do it, in fewer lines of code.
The problem was from this previous question on Stack Overflow.
My solution:
/// <summary>
/// For a given square number, tells you the corresponding row and column numbers.
/// Pre: none.
/// Post: returns the row and column numbers, via "out" parameters.
/// </summary>
/// <param name="squareNumber">The input square number.</param>
/// <param name="rowNumber">The output row number.</param>
/// <param name="columnNumber">The output column number.</param>
private static void MapSquareNumToScreenRowAndColumn(int squareNumber, out int rowNumber, out int columnNumber){
// ######################## Add more code to this method and replace the next two lines by something more sensible. ###############################
rowNumber = 0; // Use 0 to make the compiler happy for now.
columnNumber = 0; // Use 0 to make the compiler happy for now.
columnNumber = squareNumber % 6;
rowNumber = squareNumber/6;
//switch statement for assigning column values to odd rows
int remainder = squareNumber%6;
switch (remainder){
case 0:
columnNumber = 5;
break;
case 1:
columnNumber = 4;
break;
case 2:
columnNumber = 3;
break;
case 3:
columnNumber = 2;
break;
case 4:
columnNumber = 1;
break;
case 5:
columnNumber = 0;
break;
}
if (squareNumber >=0 && squareNumber <= 5){
rowNumber = 6;
columnNumber = squareNumber%6;
}
if (squareNumber >=6 && squareNumber <= 11){
rowNumber = 5;
}
if (squareNumber >=12 && squareNumber <= 17){
rowNumber = 4;
columnNumber = squareNumber%6;
}
if (squareNumber >=18 && squareNumber <= 23){
rowNumber = 3;
}
if (squareNumber >= 24 && squareNumber <= 29){
rowNumber = 2;
columnNumber = squareNumber%6;
}
if (squareNumber >= 30 && squareNumber <= 35){
rowNumber = 1;
}
if (squareNumber >= 36 && squareNumber <= 41){
rowNumber = 0;
columnNumber = squareNumber % 6;
}
}//end MapSquareNumToScreenRowAndColumn
I’m fairly new to programming, so I just wanted a second opinion on it. Is there a way for me to get the same outcome, but with less code?
Solution
The problem of your code is that you use separate conditions for each particular row and for each particular column.
But it can be easily aggregated into one – check if the row number is odd. A rest of the desired functionality can be reached using arithmetics.
Please try this code:
private static void MapSquareNumToScreenRowAndColumn(int squareNumber, out int rowNumber, out int columnNumber)
{
const int Rows = 7;
const int Cols = 6;
// Calculate the rowNumber and the columnNumber at once.
// See Math.DivRem for details: http://msdn.microsoft.com/en-us/library/yda5c8dx(v=vs.110).aspx
rowNumber = Rows - 1 - Math.DivRem(squareNumber, Cols, out columnNumber);
// If the rowNumber is odd:
if ((rowNumber & 1) != 0) // (rowNumber & 1) is the same as (rowNumber % 2)
columnNumber = Cols - 1 - columnNumber; // Invert the columnNumber
}
I know you’ve already received an answer to your question, but I believe you could benefit from some feedback on the code you wrote.
You’ve numbered your squares oddly, and I believe that is the source of your problems. The squares in your grid should be referenced using (x,y) co-ordinates. Or row and column if you prefer. Storing your squares in a 2d array would probably make this entire function obsolete.
However, I do see some issues within the code. The first of which is this.
// ######################## Add more code to this method and replace the next two lines by something more sensible. ###############################
rowNumber = 0; // Use 0 to make the compiler happy for now.
columnNumber = 0; // Use 0 to make the compiler happy for now.
Those comments are obsolete. Remove them. They only serve to confuse the maintainer 6 months from now. The other issue there is that you really shouldn’t be writing code just to “make the compiler happy”.
The other big issue I see if your use of 6 as a magic number. You’ve effectively hardcoded yourself into a corner. What if you sometime later wish to make your grid larger, or support multiple sized grids? You would have to make changes to no less than 5 lines of the code. Minimally, you should create a gridSize constant and prefer it over the hardcoded number six. This would allow you to make one change that cascades through the code where ever gridSize appears.
With that all said, you are doing something very right. Your style and naming is great. The only thing I would change there is rowNumber and columnNumber. I usually advise against shortening variable names, but I find those to be overly verbose. row and column would be just as crystal clear to the maintainer.