2
\$\begingroup\$

I wrote this code as a refactor of code I saw on Programiz. I thought about this refactor before going to bed and implemented it successfully in code.

// Make an array of suits
const suits = "S D C H".split(' ')

// Make an array of values
const values = "A 2 3 4 5 6 7 9 10 J Q K".split(' ')


// Create a function to pick two random cards
function pickTwoRandomCards(suit, value) {
  randomSuitOne = suit[Math.floor(Math.random() * suit.length)]
  randomValueOne = value[Math.floor(Math.random() * value.length)]

  randomSuitTwo = suit[Math.floor(Math.random() * suit.length)]
  randomValueTwo = value[Math.floor(Math.random() * value.length)]

  // Check to see if random cards happen to match
  // if they match run the function recursively because you can't pick the same
  // card from a deck twice
  if (randomValueOne + randomSuitOne == randomValueTwo + randomSuitTwo) {
    pickTwoRandomCards(suit, value);
  } else {
    return `${randomValueOne}${randomSuitOne} ${randomValueTwo}${randomSuitTwo}`
  }
}

const pickTwo = pickTwoRandomCards(suits, values)
console.log(pickTwo)

\$\endgroup\$
1
  • 1
    \$\begingroup\$ a refactor of code I saw on Programiz => refactoring by definition means changing code without changing behavior. This version is not rearranging an array of 52 cards. Programiz inspired this program certainly but it does not refactor that program. \$\endgroup\$
    – radarbob
    Sep 22, 2022 at 0:57

1 Answer 1

3
\$\begingroup\$

There is a bug

A return statement is missing when making the recursive call when the same card is picked twice.

Don't use recursion to repeat an operation

To repeat an operation, use a loop, for example:

while (true) {
  // ...

  if (randomValueOne + randomSuitOne != randomValueTwo + randomSuitTwo) {
    return `${randomValueOne}${randomSuitOne} ${randomValueTwo}${randomSuitTwo}`
  }
}

Picking two random cards without duplicates and without an infinite loop

There is an interesting idea in a famous answer, I quote the relevant part verbatim:

initialize set S to empty
for J := N-M + 1 to N do
    T := RandInt(1, J)
    if T is not in S then
        insert T in S
    else
        insert J in S

assumes that RandInt(1, J) returns values inclusive of J.

To understand the above algorithm, you need to realize that you choose a random value from a range that is smaller than the full range, and then after each value, you extend that to include one more. In the event of a collision, you can safely insert the max because it was never possible to include it before.

If we can use a single random number to decide both the suit and the value, then we can use the above technique to generate two distinct random numbers from the possible choices, without looping. Putting that into code:

function pickTwoRandomCards(suits, values) {
  const max = suits.length * values.length
  const random1 = Math.floor(Math.random() * (max - 1))
  const random2maybe = Math.floor(Math.random() * max)
  const random2 = random2maybe != random1 ? random2maybe : max - 1

  const randomSuitOne = suits[Math.floor(random1 / values.length)]
  const randomValueOne = values[random1 % values.length]

  const randomSuitTwo = suits[Math.floor(random2 / values.length)]
  const randomValueTwo = values[random2 % values.length]

  return `${randomValueOne}${randomSuitOne} ${randomValueTwo}${randomSuitTwo}`
}

Fix variable declarations

randomSuitOne and the other variables inside the function are missing the const keyword.

Consider usability

Returning two random cards separated by a space doesn't seem very useful. It would be good to consider how the returned values are expected to be used by callers.

Confusing function parameters suit and value

It's confusing that these parameters are arrays, but their names are singular.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.