4
\$\begingroup\$

I am building a little helper for the card game Bridge. The aim is to generate a "hand" of 13 randomly-dealt cards, and then to run various calculations on the hand to help a player with the "bidding" phase of the game. (I'm hoping knowledge of Bridge is not required for this code review!)

You will see that after creating the deck of cards and dealing 13 cards to make the hand, I put each suit's cards into a class for that suit. This enables me to deal with each suit's cards individually but also create global calculations that apply to the whole hand. Is this way using best practice / the most efficient way (this is my first coding project)?

// Generate a pack of 52 cards, one of each value for each suit, into const cards

function deckBuilder() {
  const values = [
    "A",
    "2",
    "3",
    "4",
    "5",
    "6",
    "7",
    "8",
    "9",
    "10",
    "J",
    "Q",
    "K",
  ];
  const suits = ["Spades", "Hearts", "Diamonds", "Clubs"];
  const cards = [];
  for (let s = 0; s < suits.length; s++) {
    for (let v = 0; v < values.length; v++) {
      const value = values[v];
      const suit = suits[s];
      cards.push({ value, suit });
    }
  }

  return cards;
};

const cards = deckBuilder();


// create new Suit class

class Suit {
  constructor(name, major) {
    this.name = name;
    this.cards = [];
    this.major = major;
  }

  get length() {
    return this.cards.length;
  }

  get HCPs() {
    return this.calcHCPs();
  }

  get sortedCards() {
    return this.cards.sort((x, y) => {
      if ( !isNaN(x) && isNaN(y) ) {
        return 1;
     } else if ( isNaN(x) && isNaN(y) ) {
          if ( x === "J" ) { return 1 }
          else if (y === "J") { return -1 } else if ( x > y ) { return 1; }
          else { return -1; }
      } else if ( !isNaN(x) && !isNaN(y) ) {
          if ( x === "10" ) { return - 1 }
          else if (y === "10") { return 1 } else if ( x > y ) { return -1; }
          else { return 1; }
      } else {
        return -1;
      }
    });
  }

  calcHCPs() {
    let p = 0;
    for (let card of this.cards ) {
      if (card === 'A') { p += 4; }
      else if (card === 'K') { p += 3; }
      else if (card === 'Q') { p += 2; }
      else if (card === 'J') { p++; }
  //    else { break; }
    }
    return p
  }

}; 

// create the four empty suits, with properties, to be populated

const spades = new Suit('Spades', true);
const hearts = new Suit('Hearts', true);
const diamonds = new Suit('Diamonds', false);
const clubs = new Suit('Clubs', false);


// Get one deal - 13 random cards - from the deck of 52 - and push to each suit object


function randomCard(cards) {
  const random = Math.floor(Math.random() * cards.length);
  const cardValue = cards[random].value;
  const cardSuit = cards[random].suit;

  if (cardSuit === "Diamonds"){
    diamonds.cards.push(cardValue);
  } else if (cardSuit === "Hearts") {
    hearts.cards.push(cardValue);
  } else if (cardSuit === "Spades") {
    spades.cards.push(cardValue);
  } else {
    clubs.cards.push(cardValue);
  }
  cards.splice(random, 1)
};


for (let counter = 0; counter < 13; counter++ ){
  randomCard(cards)
};


let shape = [ spades.length, hearts.length, diamonds.length, clubs.length ];
let shapeSorted = shape.sort().reverse();


// Calculate high card points (HCPs) 

let HCPs = spades.HCPs + hearts.HCPs + diamonds.HCPs + clubs.HCPs;

console.log("The high card points are: " + HCPs);


// DECK INFO FUNCTIONS

function isHandBalanced() {
  shape.sort().reverse();
  if ( shape[0] + shape[1] > 8 ) {
    return false;
  }
  else {
    return true;
  }
};


function suggestedBid() {
  if ( isHandBalanced() ) {
    if (HCPs < 12) { return "Pass"; }
    else if (HCPs >= 12 && HCPs <= 14) {
      if (shape[0] == 5 || shape[1] == 5) { return "You have a 5 card suit!"; } 
      else { return "1NT"; }
    }
    if (HCPs >= 15 && HCPs <= 19) { return "Open at 1 level" }
  }
  else {
    return "Not balanced";
  }
};


\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Abstractions

When coding we abstract the data we manipulate in such a way to make the code easy to write and efficient to execute. Abstracted data only contains what is relevant.

Very often coders will forget the abstraction and focus on the irrelevant details a trap you have fallen into in this case.

Abstract cards

For a deck of cards the suit, and value can be abstracted to an array of 52 values from 0 to 51 (the cardId)

The single value cardId holds all you need to...

  • get the suit cardSuit = cardId / 13 | 0; a value from 0 - 3
  • get the value cardValue = cardId % 13; a value from 0 - 12
  • display the card using the cardId to locate an image or use the suit and value ids to lookup a string.

The names of suits and values are only needed when you present the information to the user.

Tracking details like the cards suit and value using the real world representations just complicates the code making it hard to modify, and maintain.

A rule of coding is "Keep it Simple" and one method to simplify is via effective abstraction.

Rewrite

The following rewrite does the same as your code, with some added details.

The Deck lets you create a deck. Picked cards are random, returned cards are placed on top. The deck can be reset.

The Hand gets 13 cards from a deck, displays the cards in the console and uses your method to calculate the bid.

The are 12 lines of code that are not needed, but I left them in to illustrate high level usage. Eg display cards, manage deck, pick random cards.

const VALS = "2,3,4,5,6,7,8,9,10,J,Q,K,A".split(",");
const SUITS = "♠,♡,♣,♢".split(",");
const VAL_HCP = [0,0,0,0,0,0,0,0,0,1,2,3,4];

console.log("Hint: '" + Hand(Deck()) + "'")
function Deck() {
    const cards = new Array(52).fill(0).map((v, i)=> i);
    return {
        get remaining() { return cards.length },
        get card() { return cards.splice(Math.random() * cards.length | 0, 1)[0] },
        set card(card) { cards.push(card) },
        reset() { cards.length = 52; cards.map((v, i) => i) },
    };
};
function Shuffle(deck) { // example using deck to shuffle cards
    const shuffled = [];
    while (deck.remaining) { shuffled.push(deck.card) }
    while (shuffled.length) { deck.card = shuffled.pop() }
    return deck;        
}
function Hand(deck) {
    const cards = [];
    while (cards.length < 13) { cards.push(deck.card) }
    const getSuit = suitIdx => cards.filter(c => (c / 13 | 0) === suitIdx);
    const suits = [getSuit(0), getSuit(1), getSuit(2), getSuit(3)];

    const suitHCP = (suit) => suit.reduce((hcp, c) => hcp + VAL_HCP[c % 13], 0);
    const HCPs = suits.reduce((hcps, suit) => hcps + suitHCP(suit), 0);
    const shapes = suits.map(suit => suit.length).sort((a,b)=>a-b);

    console.log(cards.sort((a,b)=>a-b).map(c => SUITS[c/13 | 0] + VALS[c%13]).join(", ") + " HCPS: " + HCPs);

    if (shapes[2] + shapes[3] > 8) {
        if (HCPs < 12) { return "Pass" }
        if (HCPs >= 12 && HCPs <= 14) {
            if (shapes[2] === 5 || shapes[3] === 5) { return "You have a 5 card suit!" }
            return "1NT";
        }
        if (HCPs >= 15 && HCPs <= 19) { return "Open at 1 level" }
    } else { return "Not balanced" }
    return "Unknown Play";
}

\$\endgroup\$
2
  • \$\begingroup\$ I'm sorry, but I don't believe representing a card using just a integer is readable or "simple" (unless maybe it is completely hidden inside a card class, which it isn't). IMO having separate suit and value fields is a clear one-to-one representation of the real object which is much more obvious and comprehensible. \$\endgroup\$
    – RoToRa
    Commented Jun 27, 2021 at 20:59
  • 1
    \$\begingroup\$ @RoToRa I must totally disagree as high level abstractions are a primary source of overly complex source code and poorly performing execution. Look at OPs getter Suit.sortedCards 16 lines of code that can be done in one this.card.sort((a,b)=>a-b) in a fraction of the time. Coders are the smart ones! Why does the coder need to know the SUIT and VALUE as irrelevant named items? They relational entities, "is value greater / same / less", "is suit greater / same / less?" \$\endgroup\$
    – Blindman67
    Commented Jun 28, 2021 at 12:10

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.