Skip to main content
added 215 characters in body
Source Link
Pierre Menard
  • 2.2k
  • 15
  • 30

The code block

Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
Boolean loop = false; 
if(!added.add(index)) loop = true;
while(loop)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if(added.add(index)) loop = false;
}

Contains several lines of duplicated code. This pattern can often be simplified by removing the code outside of the while block:

Boolean success = false; 
while (!success)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        success = true;
}

or a do/while block, which is more idiomatic for code that needs to execute at least once:

Boolean success = false;
do
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    success = added.add(index); // note: this is simpler than the if version
}
while (!success);

I'm not a huge fan of while (true), but some people might prefer the simpler:

while (true)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        break;
}

This is all if you want to simply improve the existing structure. janos is correct that if all you want is a shuffling of a range of numbers you should approach it from that direction. Your current solution can take an unbounded amount of time to run, if the random number generator is "unlucky". You are returning a Set, which generally doesn't have a predictable order. You happen to use an implementation that does preserve insert order, but callers of you code have now way of knowing that. I would return a data structure that implies ordering, like a List. Internally you used a Set to have it prevent duplicates, but if you shuffle that's not functionality you need.

A final nit: added isn't a great name for the set that you are going to return. I often use ret to name the variable that will be the return value, though some might prefer a more descriptive name like set.

The code block

Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
Boolean loop = false; 
if(!added.add(index)) loop = true;
while(loop)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if(added.add(index)) loop = false;
}

Contains several lines of duplicated code. This pattern can often be simplified by removing the code outside of the while block:

Boolean success = false; 
while (!success)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        success = true;
}

or a do/while block, which is more idiomatic for code that needs to execute at least once:

Boolean success = false;
do
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    success = added.add(index); // note: this is simpler than the if version
}
while (!success);

I'm not a huge fan of while (true), but some people might prefer the simpler:

while (true)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        break;
}

A final nit: added isn't a great name for the set that you are going to return. I often use ret to name the variable that will be the return value, though some might prefer a more descriptive name like set.

The code block

Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
Boolean loop = false; 
if(!added.add(index)) loop = true;
while(loop)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if(added.add(index)) loop = false;
}

Contains several lines of duplicated code. This pattern can often be simplified by removing the code outside of the while block:

Boolean success = false; 
while (!success)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        success = true;
}

or a do/while block, which is more idiomatic for code that needs to execute at least once:

Boolean success = false;
do
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    success = added.add(index); // note: this is simpler than the if version
}
while (!success);

I'm not a huge fan of while (true), but some people might prefer the simpler:

while (true)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        break;
}

This is all if you want to simply improve the existing structure. janos is correct that if all you want is a shuffling of a range of numbers you should approach it from that direction. Your current solution can take an unbounded amount of time to run, if the random number generator is "unlucky". You are returning a Set, which generally doesn't have a predictable order. You happen to use an implementation that does preserve insert order, but callers of you code have now way of knowing that. I would return a data structure that implies ordering, like a List. Internally you used a Set to have it prevent duplicates, but if you shuffle that's not functionality you need.

A final nit: added isn't a great name for the set that you are going to return. I often use ret to name the variable that will be the return value, though some might prefer a more descriptive name like set.

added 215 characters in body
Source Link
Pierre Menard
  • 2.2k
  • 15
  • 30

The code block

Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
Boolean loop = false; 
if(!added.add(index)) loop = true;
while(loop)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if(added.add(index)) loop = false;
}

Contains several lines of duplicated code. This pattern can often be simplified by removing the code outside of the while block:

Boolean success = false; 
while (!success)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        success = true;
}

or a do/while block, which is more idiomatic for code that needs to execute at least once:

Boolean success = false;
do
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    success = added.add(index); // note: this is simpler than the if version
}
while (!success);

I'm not a huge fan of while (true), but some people might prefer the simpler:

while (true)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        break;
}

A final nit: added isn't a great name for the set that you are going to return. I often use ret to name the variable that will be the return value, though some might prefer a more descriptive name like set.

The code block

Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
Boolean loop = false; 
if(!added.add(index)) loop = true;
while(loop)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if(added.add(index)) loop = false;
}

Contains several lines of duplicated code. This pattern can often be simplified by removing the code outside of the while block:

Boolean success = false; 
while (!success)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        success = true;
}

or a do/while block, which is more idiomatic for code that needs to execute at least once:

Boolean success = false;
do
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    success = added.add(index); // note: this is simpler than the if version
}
while (!success);

I'm not a huge fan of while (true), but some people might prefer the simpler:

while (true)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        break;
}

The code block

Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
Boolean loop = false; 
if(!added.add(index)) loop = true;
while(loop)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if(added.add(index)) loop = false;
}

Contains several lines of duplicated code. This pattern can often be simplified by removing the code outside of the while block:

Boolean success = false; 
while (!success)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        success = true;
}

or a do/while block, which is more idiomatic for code that needs to execute at least once:

Boolean success = false;
do
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    success = added.add(index); // note: this is simpler than the if version
}
while (!success);

I'm not a huge fan of while (true), but some people might prefer the simpler:

while (true)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        break;
}

A final nit: added isn't a great name for the set that you are going to return. I often use ret to name the variable that will be the return value, though some might prefer a more descriptive name like set.

added 123 characters in body
Source Link
Pierre Menard
  • 2.2k
  • 15
  • 30

The code block

Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
Boolean loop = false; 
if(!added.add(index)) loop = true;
while(loop)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if(added.add(index)) loop = false;
}

Contains several lines of duplicated code. This pattern can often be simplified with aby removing the code outside of the while block:

Boolean success = false; 
while (!success)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        success = true;
}

or a do/while block, which is more idiomatic for code that needs to execute at least once:

Boolean success = false;
do
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    success = added.add(index); // note: this is simpler than the if version
}
while (!success);

I'm not a huge fan of while (true), but some people might prefer the simpler:

while (true)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        break;
}

The code block

Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
Boolean loop = false; 
if(!added.add(index)) loop = true;
while(loop)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if(added.add(index)) loop = false;
}

Contains several lines of duplicated code. This pattern can often be simplified with a while block:

Boolean success = false; 
while (!success)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        success = true;
}

or a do/while block:

Boolean success = false;
do
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    success = added.add(index); // note: this is simpler than the if version
}
while (!success);

I'm not a huge fan of while (true), but some people might prefer the simpler:

while (true)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        break;
}

The code block

Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
Boolean loop = false; 
if(!added.add(index)) loop = true;
while(loop)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if(added.add(index)) loop = false;
}

Contains several lines of duplicated code. This pattern can often be simplified by removing the code outside of the while block:

Boolean success = false; 
while (!success)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        success = true;
}

or a do/while block, which is more idiomatic for code that needs to execute at least once:

Boolean success = false;
do
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    success = added.add(index); // note: this is simpler than the if version
}
while (!success);

I'm not a huge fan of while (true), but some people might prefer the simpler:

while (true)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        break;
}
Source Link
Pierre Menard
  • 2.2k
  • 15
  • 30
Loading