Code Review

Welcome to Code Review Stack Exchange

Code Review is a question and answer site for seeking peer review of your code. It's built and run by you as part of the Stack Exchange network of Q&A sites. We're working together to improve the skills of programmers worldwide by taking working code and making it better.

Before posting a question, please read What topics can I ask about here? and How do I ask a good question?.

To get you the best answers, we’ve provided some guidance:
1. Code must produce the desired output
2. Code must not error or contain known bugs
3. Titles should describe what the code does
4. The more code you provide the more we can help
5. After an answer is posted, you must not edit your question to invalidate any advice.

We're a little bit different from other sites. Here's how:

Ask questions, get answers, no distractions

This site is all about getting answers. It's not a discussion forum. There's no chit-chat.

Just questions...

...and answers.

up vote

Good answers are voted up and rise to the top.

The best answers show up first so that they are always easy to find.

accept

The person who asked can mark one answer as "accepted".

Accepting doesn't mean it's the best answer, it just means that it worked for the person who asked.

Generate random numbers without repetitions

up vote 14 down vote

I want to generate a list of N different random numbers:

public static List<int> GetRandomNumbers(int count)
{
    List<int> randomNumbers = new List<int>(); 

    for (int i=0; i<count; i++) 
    {    
        int number;

        do number = random.Next();
        while (randomNumbers.Contains(number));

        randomNumbers.Add(number);
    }

    return randomNumbers;
}

But I feel there is a better way. This do...while loop especially makes this ugly. Any suggestions on how to improve this?

2 Answers

up vote 4 down vote accept

The algorithm looks fine to me. It shows no clear flaws, and will work fine in common use cases.
For extreme cases of selecting most of the numbers in a very large interval, it will be very inefficient. But unless that is an expected use case, I would not bother changing the code to more complicated approaches.

But I suggest you always use explicit braces around blocks even when they have a single statement:

do
{
    bar();
}
while (foo);

I would also rename the method to GetNRandomNumbers, to make the method's name match its purpose better.

up vote 3 down vote

Another solution is to use a Format Preserving Encryption cipher (such as AES using the FFX mode), configured to map from 32 bit integers to 32 bit integers.

Seed it randomly, then simply encrypt the first 'count' integers.

These encrypted numbers will be as random as the seed, and won't repeat.

Get answers to practical, detailed questions

Focus on questions about an actual problem you have faced. Include details about what you have tried and exactly what you are trying to do.

Ask about...

The quality of your working code with regards to:

  • Best practices and design pattern usage
  • Security issues
  • Performance
  • Correctness in unanticipated cases

Not all questions work well in our format. Avoid questions that are primarily opinion-based, or that are likely to generate discussion rather than answers.

Questions that need improvement may be closed until someone fixes them.

Don't ask about...

  • Troubleshooting, debugging, or understanding code snippets
  • How to add a feature
  • How to fix compile-time errors, runtime errors, or incorrect results
  • Best practices in general (that is, it's okay to ask "Does this code follow common best practices?", but not "What is the best practice regarding X?")
  • Tools, improving, or conducting code reviews
  • Higher-level architecture and design of software systems

Tags make it easy to find interesting questions

All questions are tagged with their subject areas. Each can have up to 5 tags, since a question might be related to several subjects.

Click any tag to see a list of questions with that tag, or go to the tag list to browse for topics that interest you.

Generate random numbers without repetitions

up vote 14 down vote

I want to generate a list of N different random numbers:

public static List<int> GetRandomNumbers(int count)
{
    List<int> randomNumbers = new List<int>(); 

    for (int i=0; i<count; i++) 
    {    
        int number;

        do number = random.Next();
        while (randomNumbers.Contains(number));

        randomNumbers.Add(number);
    }

    return randomNumbers;
}

But I feel there is a better way. This do...while loop especially makes this ugly. Any suggestions on how to improve this?

You earn reputation when people vote on your posts

Your reputation score goes up when others vote up your questions, answers and edits.

+10 question voted up
+10 answer voted up
+15 answer is accepted
+2 edit approved

As you earn reputation, you'll unlock new privileges like the ability to vote, comment, and even edit other people's posts.

Reputation Privilege
15 Vote up
50 Leave comments
125 Vote down (costs 1 rep on answers)

At the highest levels, you'll have access to special moderation tools. You'll be able to work alongside our community moderators to keep the site focused and helpful.

2000 Edit other people's posts
3000 Vote to close, reopen, or migrate questions
10000 Access to moderation tools
see all privileges

Improve posts by editing or commenting

Our goal is to have the best answers to every question, so if you see questions or answers that can be improved, you can edit them.

Use edits to fix mistakes, improve formatting, or clarify the meaning of a post.

Use comments to ask for more information or clarify a question or answer.

You can always comment on your own questions and answers. Once you earn 50 reputation, you can comment on anybody's post.

Remember: we're all here to learn, so be friendly and helpful!

up vote 9 down vote

The algorithm looks fine to me. It shows no clear flaws, and will work fine in common use cases.
For extreme cases of selecting most of the numbers in a very large interval, it will be very inefficient. But unless that is an expected use case, I would not bother changing the code to more complicated approaches.

But I suggest you always use explicit braces around blocks even when they have a single statement:

do
{
    bar();
}
while (foo);

I would also rename the method to GetNRandomNumbers, to make the method's name match its purpose better.

edit

@FlorianF this is Code Review, not Stack Overflow. Reviewers are always free to comment on any aspect of the code. Always. - Mathieu Guindon Sep 4 '14 at 23:44

add a comment

Unlock badges for special achievements

Badges are special achievements you earn for participating on the site. They come in three levels: bronze, silver, and gold.

In fact, you can earn a badge just for reading this page:

 Informed Read the entire tour page
 Student First question with score of 1 or more
 Editor First edit
 Good Answer Answer score of 25 or more
 Civic Duty Vote 300 or more times
 Famous Question Question with 10,000 views

see all badges

Sign up to get started

Sign up

Signing up allows you to:

  • Earn reputation when you help others with questions, answers and edits.
  • Select favorite tags to customize your home page.
  • Claim your first badge:  Informed
Looking for more in-depth information on the site? Visit the Help Center

Code Review Stack Exchange is part of the Stack Exchange network

Like this site? Stack Exchange is a network of 177 Q&A sites just like it. Check out the full list of sites.

Stack Exchange

Your privacy

By clicking “Accept all cookies”, you agree Stack Exchange can store cookies on your device and disclose information in accordance with our Cookie Policy.