Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review: MoreEnumerable.Random() #33

Closed
GoogleCodeExporter opened this issue Mar 23, 2015 · 7 comments
Closed

Review: MoreEnumerable.Random() #33

GoogleCodeExporter opened this issue Mar 23, 2015 · 7 comments

Comments

@GoogleCodeExporter
Copy link

Purpose of code changes on this branch:

Implementation of a set of operators that allow generating sequences of
random numbers. Overloads are available that map to the Next() and
NextDouble() methods of System.Random. All overloads of Random() are
intended to be lazy, streaming generators that produce an infinite sequence
of random numbers (compliant with the constraints placed by the particular
overload used). 

When reviewing my code changes, please focus on:

* The public interface of the extension methods.
* Their compatibility with typical use and public interface of the
System.Random class.
* The effectiveness and clarity of the available XML comment documentation.
* How well these operators fit will into the MoreLINQ ecosystem of
extension methods.
* The correctness of the implementation and the extent of coverage
available from the corresponding unit tests.

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by ambientl...@gmail.com on 17 Jan 2010 at 3:56

@GoogleCodeExporter
Copy link
Author

Original comment by azizatif on 17 Jan 2010 at 11:22

@GoogleCodeExporter
Copy link
Author

Thank you for the code review. In response to your comments:

1. Regarding UTF-8 signature - VisualStudio is typically configured to create 
.cs
files as UTF-8. Other files in MoreLINQ also appear to be UTF-8, so hopefully 
what
I'm committing is consistent. If there's a different default encoding, please 
let me
know - and I will update my settings so as to be consistent.

2. I agree with the suggestion for the improved error message. I will update 
the code
to reflect your proposed wording.

3. My personal coding style has been to use Pascal case to identify delegates 
passed
to methods - I find it to be less confusing. It also makes the call site look 
more
like a function call. However, it does appear that most other operators in 
MoreLINQ
use camel case for delegates, so I will update my implementation to be 
consistent.

Original comment by ambientl...@gmail.com on 17 Jan 2010 at 11:45

@GoogleCodeExporter
Copy link
Author

> VisualStudio is typically configured to create .cs
> files as UTF-8.

Interesting. Looks like mine is not typically configured. Point 1 is not a big 
problem. Just thought I'd point it out. I usually work a lot on the command 
line so I 
notice those signature characters a lot where they don't get always filtered 
out.

3. I think this will become more pronounced with named arguments being 
introduced 
with C# 4.0 so better to address it now.

Otherwise, great stuff!

Original comment by azizatif on 18 Jan 2010 at 7:00

@GoogleCodeExporter
Copy link
Author

Original comment by azizatif on 23 Jan 2010 at 11:16

  • Changed state: Started

@GoogleCodeExporter
Copy link
Author

Now migrated to Hg and available in clone:
http://code.google.com/r/azizatif-morelinq-evenmore/
This clone is ready to be cloned :O) for further review. It also has the 
main/default branch already merged in to bring it up to date.

Original comment by azizatif on 25 May 2012 at 11:38

@GoogleCodeExporter
Copy link
Author

Following no response from ambientlion (who possibly/assuming lost interest 
given it has taken years to get this far), taking ownership of issue to 
consider as addition for MoreLINQ 2.0 milestone.

Original comment by azizatif on 12 Jun 2013 at 10:10

@GoogleCodeExporter
Copy link
Author

This issue was closed by revision ecbb044b3f86.

Original comment by azizatif on 14 Jun 2013 at 5:18

  • Changed state: Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant