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 have two snippets of code that I would like you to look at. I can't figure out which of the two is most efficient (or better practice, if applicable). I'm currently finding the row and column of a sprite in a sprite sheet in order to render it to the screen; which works find and dandy. However, I now have two algorithms:

const double Testvalue = 155.0f; //This values changes, could be any natural number.
const double imageWidth = 614.0f; 
const double SpriteAverageSize = 34.0f; //floor(value / spriteAverageSize) gives us max rows (which is this case is 18).

double numRows = floor(imageWidth / SpriteAverageSize);
double RowValue = ceil(Testvalue / numRows);
double ColumnValue = numRows - ((numRows  * RowValue) - Testvalue);

//This will do the following operation: 
//numRows = floor(614.0/ 34.0) -> floor(18.05..) -> 18.0.
//RowValue = ceil(155.0/ 18.0) -> ceil(8.611..) -> 9.0.
//ColumnValue = 18.0 - ((18.0 * 9.0) - 155.0) -> 18.0 - (162.0 - 155.0) -> 18.0 - 7.0 -> 11.0.

//Row: 9 ; Column: 11.

or

const double Testvalue = 155.0f;
const double imageWidth = 614.0f;
const double SpriteAverageSize = 34.0f;

double RowValue = ceil(Testvalue / floor(imageWidth / SpriteAverageSize));
double ColumnValue = floor(imageWidth / SpriteAverageSize) - ((floor(imageWidth / SpriteAverageSize) * RowValue) - Testvalue);

//This will do the following operation:
//RowValue = ceil(155.0 / floor(614.0 / 34.0)) -> ceil(155.0 / floor(18.05..)) -> ceil(155.0 / 18.0) -> ceil(8,61..) -> 9.0.
//ColumnValue = floor(614.0 / 34.0) - ((floor(614.0 / 34.0) * 9.0) - 155.0) -> floor(18.05..) - ((floor(18.05..) * 9.0) - 155.0) -> 18.0 - ((18.0 * 9.0) - 155.0) -> 18.0 - 7.0 -> 11.0.

//Row: 9 ; Column: 11

To clarify, these two work perfectly (I have also implemented it as a method in my class to do the operation as I get the data from an XML file), I get the right row and column value as I've tested several times and implementing it works too. I would just like some tips on which of the two algorithms you'd prefer in a block of code and which of the two is more efficient?

share|improve this question
    
I am down-voting this because it's a) An "A or B?"-style question and b) because it is technically the exact same code (which you don't seem to be aware of), the only difference is that you have just extracted a variable in the first version. –  Simon André Forsberg Mar 2 at 15:42
3  
@Sean The problem with "Is A or B better?" questions is that sometimes the answer might be C. If you posted more of the code people might find other ways to improve it. For example, from the posted code fragment I couldn't tell how Testvalue was set (it's defined as const but the comment says it changes); and I couldn't tell which variables are defined at global scope, and which are local variables. –  ChrisW Mar 2 at 16:09
    
@ChrisW My question wasn't focused on improving the code, I simply wanted to know which of A or B a coder would prefer in their code. More code isn't necessary as what it does here is what it does in my software. Testvalue is set as const as the actual values would be gotten from a const vector<vector<int>> foo, so yes, the values do change as it traverses the 2D vector. I also specified at the bottom that it is implemented in a class, so it would be local variables to the class. However, I do understand where you are coming from. –  Sean Mar 2 at 16:15
2  
There seems to be a misunderstanding about what is considered to be on-topic for CodeReview here. It is clear in the help center that you should expect more than an A/B answer. When you asked your question you are saying yes to: Do I want feedback about any or all facets of the code?. No point in blaming other people for doing what you asked them to do –  rolfl Mar 2 at 16:24

1 Answer 1

up vote 4 down vote accepted

I prefer the first one, because having the named variable numRows:

  • Certainly makes the code more "self-documenting": it's easier for me (the programmer) to understand what the algorithm is doing
  • May make the code faster: because you only do the floor(imageWidth / SpriteAverageSize) calculation once instead of three times (although it's possible that an optimizing compiler might be able to make that optimization itself, even without your 'hint' that it's a reused sub-expression)
share|improve this answer
    
Thank you. That was very helpful. It makes sense for the first algorithm to be a more popular choice. It is definitely more self-documenting. –  Sean Mar 2 at 15:39

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.