Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am using C# to get the user's input of how many buttons to change the color of. The buttons are in rows that have 1-10 button in each. If the user enters 11, then the entire first row will be filled out and the first button on the second row will change color. I have figured out how to accommodate for different numbers the user might enter, but hoping to get some advice on how to code better without using a large if, if else, statement.

internal void ChangeTileColor(ConnectorData connectorData, int j)
{
    int row = 0;
    if (j < 10)
    {

    }
    else if (j < 20)
    {
        j -= 10;
        row++;
    }
    else if (j < 30)
    {
        j -= 20;
        row += 2;
    }
    else if (j < 40)
    {
        j -= 30;
        row += 3;
    }
    else if (j < 50)
    {
        j -= 40;
        row += 4;
    }
    else if (j < 60)
    {
        j -= 50;
        row += 5;
    }
    Button button = this.tableLayoutPanel1.GetControlFromPosition(j, row) as Button;
    button.BackColor = Color.Lime;
}
share|improve this question

1 Answer 1

up vote 10 down vote accepted

This one's pretty straightforward. If you have rows of length rowLength, then the row is j / rowLength (note that with integer arithmetic this automatically rounds down to the nearest integer) and the column is j % rowLength.


Altough there was a very simple answer in this particular case, in general, if you look at a method like the one you wrote, the very repetitive code should stand out, and you should think "loop".

In fact, a good starting point is to just say, in plain English, what you're doing with what you've written. Something like:

Starting from 10, and adding in increments of 10, I check if j is less than that number. If it is, I subtract the previous multiple of 10 from j, and add a tenth of that number to row. Otherwise I go on to check the next increment of 10.

That's a start, but a good general idea with programming is to say what you're doing, not how you're doing it. So the above might become:

In order to check which row the item is in, I check each multiple of the row count, starting from the first, until I find the first one greater than the position. Then I record that the number of the row is that multiple - 1. Then I know that the column is the position minus the number of items in all the previous rows, which is the number of previous rows multiplied by the number of items per row.

A bit cumbersome in English, but the above is actually the exact basis of what you did. So the next task is to translate that as plainly into code as possible.

First, we look at "I check each multiple". That doesn't say "I check 1 * the row count, then I check 2 * the row count, then I check 3 * the row count". It expresses all of those in a single statement: each. So translating that as literally into code as possible, it's not a bunch of if statements, it's a single loop:

int i = 0; //Starting from the first...
do
{
    i++; //I check each...
    int rowMultiple = i*rowLength; //...Multiple of the row length
} while(true);  //Not sure when I stop yet

Next: "until I find the first one greater than the position":

int i = 0; //Starting from the first...
do
{
    i++; //I check each...
    int rowMultiple = i*rowLength; //...Multiple of the row length
} while(rowMultiple <= position);  //Until I find the first one greater than the position

Note that I renamed j to position, since that's what it is. Now we've found the desired i, and we know that to do with it:

int i = 0; //Starting from the first...
do
{
    i++; //I check each...
    int rowMultiple = i*rowLength; //...Multiple of the row length
} while(rowMultiple <= position);  //Until I find the first one greater than the position
int rowNumber = i - 1; //Then I know the row number is this multiple minus 1
int columnNumber = position - (rowNumber*rowLength); //And the column number is the position minus the number of items in previous rows

And there you have it! Of course you wouldn't really include those comments, those are just for in-line illustrations of how directly each line- in code- corresponds to what you're actually trying to do- in English. The fact that each line so clearly describes what it's doing without comments is a sign that you're on the right track.


From there, the solution with / and % is really just noticing that you can predict what the loop will produce without actually having to go through each iteration, and likewise knowing about the modulo (%) operator and thinking to use it, to simplify the column bit. But being able to clearly state what you want to do in English and turn that into concise, readable code is a far more general and wide-reaching skill.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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