added monte carlo method c# #239
Conversation
june128
added
the
Implementation
| } | ||
|
|
||
|
|
||
| private static double MonteCarloMethod(int samples) |
There was a problem hiding this comment.
I would move this to a separate file called MonteCarlo with the class MonteCarlo and rename the method into Run(), since that's the way all other C# implementations work (look here for reference https://www.algorithm-archive.org/chapters/computational_geometry/gift_wrapping/jarvis_march/jarvis_march.html).
|
Two other things:
|
| var randomY = random.NextDouble() * radius; | ||
|
|
||
| if (IsInCircle(randomX, randomY)) | ||
| { |
There was a problem hiding this comment.
Since this is just a single line, you don't need the curly braces. I would remove them, what do you think?
|
|
||
|
|
||
| private static bool IsInCircle(double x, double y) | ||
| { |
There was a problem hiding this comment.
Here you can use => sine you just have a single line in the method. So the method would roughly look like this
private static bool IsInCircle(double x, double y) => x * x + y * y < radius * radius;It's also just a suggestion and I would like to hear your input.
|
Ok, I reviewed and commented quite a few things now. But I also want to welcome you to the AAA community! If you have any questions, suggestion or concerns feel free to publish them here or on our Discord (depending on situation): https://discord.gg/pb976sY |
|
Ohh and another final request: Please change the name of the directory you put the code in from |
|
Hi. Thanks for the review. I have moved code to MonteCarlo.cs, changed dir name to csharp, updated .md files and used method expression. |
| @@ -0,0 +1,18 @@ | |||
| using System; | |||
|
|
|||
| // submitted by vitalii shvetsov (@sanstorik) | |||
There was a problem hiding this comment.
Maybe move this at the very top?
| @@ -123,6 +126,8 @@ Feel free to submit your version via pull request, and thanks for reading! | |||
| {% sample lang="swift" %} | |||
| ### Swift | |||
| [import, lang:"swift"](code/swift/monte_carlo.swift) | |||
| ### CSharp | |||
There was a problem hiding this comment.
You need to put {% sample lang="csharp" %} above this line, so that we see the code when selecting C#.
| @@ -59,8 +59,11 @@ each point is tested to see whether it's in the circle or not: | |||
| [import:13-15, lang:"java"](code/java/MonteCarlo.java) | |||
| {% sample lang="swift" %} | |||
| [import:21-25, lang:"swift"](code/swift/monte_carlo.swift) | |||
| {% sample lang="csharp" %} | |||
| [import:37-39, lang:"csharp"](code/csharp/MonteCarlo.cs) | |||
There was a problem hiding this comment.
The code import should only be line 38, since you made the method smaller.
|
Thanks for correcting these little issues. I found 3 more things, I would like to have changed, but then the PR should be ready to merge. |
|
@sanstorik How is the situation? |
|
@sanstorik I'll close this PR now and do the C# Monte Carlo implementation myself. If you're coming back, before I open my PR, feel free to tag me here and reopen this PR. |
Simple monte carlo method implementation in c#