Conversation
Spin() function rotates the wheel randomly
|
Spin() function can be important to spin wheel randomly and function can be accessed via button clicks |
jpoon
left a comment
jpoon
left a comment
There was a problem hiding this comment.
Thanks for your PR!! I dropped a couple of comments.
| get { return _backgroundColor; } | ||
| set { SetField(ref _backgroundColor, value); } | ||
| set { SetField(ref _backgroundColor, value); | ||
| Draw(); |
There was a problem hiding this comment.
Is it necessary to add the Draw()? Setting a new color should automatically be handled as this class inherits from INotifyPropertyChanged...
There was a problem hiding this comment.
Yes, i've experienced that without Draw() function call, BackgroundColor doesnot changes instantly
| /// </summary> | ||
| /// <param name="maxSpins">Maximum no. of spins or revolutions.</param> | ||
| /// <param name="durationInSec">Spin duration in Second. [-1 denotes random duration]</param> | ||
| public void Spin(int maxSpins=5, int durationInSec = -1) |
There was a problem hiding this comment.
As the Spin function is public, can you move it underneath the constructor (above the private functions).
| { | ||
| Random r = new Random(DateTime.Now.Millisecond); | ||
| var angleFromYAxis = 360 - Angle; | ||
| SelectedItem = _pieSlices |
There was a problem hiding this comment.
It looks like you are getting the currently selected pie slice? Why not use the SelectedItem getter?
| /// <param name="durationInSec">Spin duration in Second. [-1 denotes random duration]</param> | ||
| public void Spin(int maxSpins=5, int durationInSec = -1) | ||
| { | ||
| Random r = new Random(DateTime.Now.Millisecond); |
There was a problem hiding this comment.
I don't think it's necessary to pass in a seed value, the default value should be sufficient.
There was a problem hiding this comment.
Seed defines how the random numbers will be generated. As you are using DateTime.Now.Milliseconds. It's equivalent to using the default value (ie. passing in no argument).
| int fullSpin = itemIndex / count; | ||
| itemIndex = itemIndex % count; | ||
| int steps = currIndex-itemIndex; | ||
| if (steps < 0) |
|
|
||
| doubleAnimation.From = startAngle; | ||
| doubleAnimation.To = finalAngle; | ||
| if(durationInSec>0) |
There was a problem hiding this comment.
Nit, can you add { and } around the if block.
| int currIndex = _pieSlices.IndexOf(SelectedItem); | ||
| int fullSpin = itemIndex / count; | ||
| itemIndex = itemIndex % count; | ||
| int steps = currIndex-itemIndex; |
There was a problem hiding this comment.
Consider not modifying the itemIndex value as it would then no longer be "item index". Instead,
int steps = currIndex - itemIndex % count;
| if (steps < 0) | ||
| steps = count + steps; | ||
|
|
||
| var startAngle = SelectedItem.StartAngle + SelectedItem.Angle / 2; |
There was a problem hiding this comment.
Shouldn't SelectedItem.StartAngle be sufficient? Why is the .Angle/2 necessary? I'm a couple years removed from this codebase so not sure if I'm missing something.
There was a problem hiding this comment.
just to rotate the pieslice at middle :)
| doubleAnimation.From = startAngle; | ||
| doubleAnimation.To = finalAngle; | ||
| if(durationInSec>0) | ||
| doubleAnimation.Duration = new Windows.UI.Xaml.Duration(new TimeSpan(0, 0, durationInSec)); |
There was a problem hiding this comment.
Can you use TimeSpan.FromSeconds() (https://msdn.microsoft.com/en-us/library/system.timespan.fromseconds(v=vs.110).aspx) instead?
| else | ||
| doubleAnimation.Duration = new Windows.UI.Xaml.Duration(new TimeSpan(0, 0, r.Next(3, 6))); | ||
| storyBoard.Begin(); | ||
| storyBoard.Completed += StoryBoard_Completed; |
There was a problem hiding this comment.
Please set the .Completed event handler before you start the Begin() to prevent any race conditions.
There was a problem hiding this comment.
The logic for StoryBoard_Completed is fairly simple so I'd put that into an anonymous function.
|
Hey, thank you for suggestion. |
|
Please address the feedback comments I left on the last review. As I will need to maintain these changes moving forward, it is best that prior to merging this PR, the code changes meet a quality bar. |
3 participants
Spin() function rotates the wheel randomly