How to avoid side-effects (Javascript)

Question

I'm currently building a blackjack game, and one of my classes is called "Deck". This deck needs to do two things:

1. Return a set of cards that other objects can use (this is equivalent to dealing those cards out)
2. Remove those cards from the deck (once a card has been dealt, it should be removed).

At the moment, I've implemented these two pieces of functionality by creating two methods: selectCards() and removeIndexFromDeck. I first call selectCards() which returns the selected cards, but also calls removeIndexFromDeck().

Does this approach violate best practices? It looks like my selectCards() function has both a return value and side-effects.

If it does violate best practices, how would I change these methods, but make sure I'm still able to return the selected cards and also remove them from the deck.

Thanks!

class Deck {
  constructor() {
    this.deck = [];
    ['♦', '♣', '♥', '♠'].forEach(suit => {
      ['A', '2', '3', '4', '5', '6', '7', '8', '9', '10', 'J', 'Q', 'K'].forEach(value => {
        this.deck.push(`${value}${suit}`);
      });
    });
  }

  selectCards(numCards) {
    let selectedCards = [];

    while (numCards > 0) {
      let randIndex = Math.floor(Math.random() * this.deck.length);
      selectedCards.push(this.deck[randIndex]);
      this.removeIndexFromDeck(randIndex);
      numCards -= 1;
    }

    return selectedCards;
  }

  removeIndexFromDeck(index) {
    this.deck.splice(index, 1);
  }
}

Answer 1

It is true that you should generally avoid to both mutate and return a value, but there are always exceptions to good rules. Even native Java Script has exceptions to this rule, and Array#pop is probably the most well known one. Yet, many will agree that pop is very useful as it currently works. Also Array#splice, which you call in your script, mutates and returns information.

Unless you want to abandon OOP and move to functional programming, this pattern is fine. I would just make sure that the name of the method leaves as little doubt as possible about this double effect. For that reason I would call selectCards rather extractCards or pullCards. This gives a stronger hint that the deck is mutated.

I would also suggest implementing a shuffle method, instead of using random indexes at the moment of selecting cards. If you have support for private properties, then define the deck array as private, so to hide the shuffled contents from the outside world.

Here is what I mean (without private):

class Deck {
  constructor() {
    this.deck = Array.from('♦♣♥♠', suit =>
      ['A','2','3','4','5','6','7','8','9','10','J','Q','K'].map(
        value => `${value}${suit}`
      )
    ).flat();
  }

  shuffle() { // mutates the deck, much like Array#sort mutates an array
    let deck = this.deck;
    for (let i = deck.length - 1; i > 0; i--) {
      let j = Math.floor(Math.random() * (i + 1));
      let temp = deck[i];
      deck[i] = deck[j];
      deck[j] = temp;
    }
  }

  extractCards(numCards) {  // Better name. Mutates & returns. 
    // Perform a controlled Array#splice
    if (typeof numCards !== "number" || numCards <= 0) throw "Invalid argument";
    if (this.deck.length < numCards) throw "Deck does not have enough cards for this operation";
    return this.deck.splice(-numCards);
  }
}

let deck = new Deck();
deck.shuffle();
console.log(...deck.extractCards(4));

Answer 2

If you're using classes, that usually implies there are side effects, since the data is part of the class. Aka you get methods that don't have return values (often meaning they have side effects) that mutate class state in place. The closest thing you can do is make as many methods as you can return new deck values, instead of using mutate/side effect array methods, and then in your parent method do this.deck = removeIndexFromDeck(index) to limit where you have side effects.

If you abandon classes and instead make functions that manipulate data, and treat deck as only data and not as a smart class, the functions themselves won't be able to have side effects.

You could also use the pattern of returning a new Deck class instance when you make a mutation, which means the data in Deck will always be immutable. I don't see a benefit of this in this case though.