Conversation
|
|
||
| int rnd(int min, int max) | ||
| { | ||
| if (min == max) return min; |
There was a problem hiding this comment.
What if min is greater than max? For example running rnd(50, 10) returned 56. Should this print an error if this occurs?
There was a problem hiding this comment.
I added a check for this edge case. I see three potential actions: throw an exception, return 0, or return rnd(max, min). I believe the third option is the best as most people who call rnd(20, 10) are expecting it to behave as rnd(10, 20) anyway. I added an output to the console whenever the function is used improperly. Throwing an exception seems extreme in this case and returning 0 could confuse users.
There was a problem hiding this comment.
Perhaps adding some form of clarification to this string would be good. Something to let the users know that we'll be swapping the values they've input into the function.
I don't think this should block this from the next review, so I'll approve this.
There was a problem hiding this comment.
The correct answer imo would be to throw an error anytime the function is being used incorrectly. However as splashkit is scared of throwing exceptions as they're too scary for people new to programming, I think this is an acceptable way of handling the error.
coresdk/src/test/test_graphics.cpp
Outdated
| save_bitmap(user_image, "1"); | ||
| fill_rectangle_on_bitmap (user_image, COLOR_BLACK, 0, 0, 10, 10); | ||
| // rnd(0) should return 0, and rnd(10, 10) should return 10 | ||
| fill_rectangle_on_bitmap (user_image, COLOR_BLACK, rnd(0), 0, rnd(10, 10), 10); |
There was a problem hiding this comment.
I don't think this is a great place for this; this should be testing graphics only. The rnd functions seem like they'd be a good for unit testing.
EDIT: Ignore this, I see you've added exactly this in PR#68. IMO this should be removed from the graphics test.
There was a problem hiding this comment.
I agree with this. I was concerned that this specific pull request would be deemed insufficient if there was no testing included. I reverted the changes to test_graphics.cpp.
Liquidscroll
left a comment
Liquidscroll
left a comment
There was a problem hiding this comment.
Great job! Left a comment about a clarification that could be made; but I don't think it should block this from the next review.
|
|
||
| int rnd(int min, int max) | ||
| { | ||
| if (min == max) return min; |
There was a problem hiding this comment.
Perhaps adding some form of clarification to this string would be good. Something to let the users know that we'll be swapping the values they've input into the function.
I don't think this should block this from the next review, so I'll approve this.
|
|
||
| int rnd(int min, int max) | ||
| { | ||
| if (min == max) return min; |
There was a problem hiding this comment.
The correct answer imo would be to throw an error anytime the function is being used incorrectly. However as splashkit is scared of throwing exceptions as they're too scary for people new to programming, I think this is an acceptable way of handling the error.
WhyPenguins
left a comment
WhyPenguins
left a comment
There was a problem hiding this comment.
Looks good! Just would be better to make the rnd error be output via the in-built logging system - after that this should be good to merge.
coresdk/src/coresdk/random.cpp
Outdated
| { | ||
| if (min > max) | ||
| { | ||
| std::cout << "Error: min value is greater than max value when calling rnd" << std::endl; |
There was a problem hiding this comment.
Just for consistency this should probably be output via LOG(WARNING) rather than cout
There was a problem hiding this comment.
I have altered the output to use LOG(WARNING). It required me to include easylogging++.h in the file.
4 participants
Description
The
rnd(int ubound)andrnd(int min, int max)overloads ofrandgenerate arithmetic exceptions when a divide by zero occurs through a modulo operator.rnd(0)will reliably generate the exception. Whenmin = max, the divide by zero will occur inrnd(int min, int max)as... % (max - min)is used in the function.Type of change
How Has This Been Tested?
I added calls to rnd(0) and rnd(10, 10) to
test_graphics.cppto test the edge cases. While it may seem out of place to userndintest_graphics.cpp,#include "random.h"was already present in the file with no associated references. Now the#includeis justified.Testing Checklist
Checklist