39
\$\begingroup\$

I was asked to write a test code for an exercise for a job I applied for.

I was rejected, and I didn't know why. And I want to ask your opinions about my code.

The exercise is like that:

Write a simple class in the language of your choice to represent a deck of cards with operations to shuffle the deck and to deal one card. While not a requirement today, a likely future enhancement is the need to deal all the cards in a deck. While not strictly required, we value usage instructions, nicely-modeled data, automated tests, and thoughtful consideration of architectural decisions and simplicity-vs-completeness trade-offs.

I chose to write the code in TypeScript, because the position is front-end developer.

So, here is my code, and you tell me what I did wrong:

in the first file : card.ts

/* *** in the file Card.ts ***** */
    // Picture cards (Jack, Queen, King and Ace) will have matching numeric rank
    enum PictureCardType {Jack = 11, Queen = 12, King = 13, Ace = 1};

    enum SuiteType { Hearts , Diamonds, Spades, Clubs};


    class Card {
        private _rank: number;
        private _type: SuiteType;
        private _isPicture = function() : boolean {
           return this._rank > 10 || this._rank === 1;
        }

        constructor(rank: number, type: SuiteType) {
            if (rank > 0 && rank < 14)
                this._rank = rank;
            else
                throw new RangeError("card rank outside the range");
            this._type = type;
        }

        get rank() : number {
            return this._rank;
        }

        get suiteType() : SuiteType {
            return this._type;
        }
        // convert to JS object that has the following format: {suite: 'Dimaonds', rank : 'Ace'}
        toJSObject(): any {
            return {suite: SuiteType[this._type], rank: this._isPicture()  ?  PictureCardType[this._rank] : this._rank.toString() };
        }

    }

    export {Card , PictureCardType, SuiteType};

In the file

/* *** CardDeck.ts **** */
import {Card, SuiteType} from './card';
import {cardShuffler} from './cardShuffler';


interface cardDeckContract {
    cards : Card[];
    shuffle(): void;
    dealCard(): Card;
}

let privateCards : Card[];

// Hey, why not use JavaScript generator to deal sequence of cards?
// This might be over-architecture, but it is a way to show the usage of generators
let getNexCard =  function*() {
    while (privateCards.length > 0) {
        let card: Card = privateCards.splice(0, 1)[0];
        yield card;
    }
};
// initiallize the generator
let cardSequence : IterableIterator<Card> = getNexCard();

// Class CardDeck is responsible of implementing the contract above: Shuffle and Deal a card

class CardDeck implements cardDeckContract {

    cardShuffler: cardShuffler;

    /// CardDeck is not the class that has shuffling algorithm.
    /// We pass the shuffling algorithm to its constructor as a simple way of Dependency Injection
    /// This decision will help us chose different shuffling ways in the future.
    /// So the CardDeck is like Strategy Pattern
    constructor(cards: Card[], cardShuffler: cardShuffler) {
        privateCards = cards.slice(0);
        // this is the shuffling utility passed to the class
        this.cardShuffler = cardShuffler;
    }

    shuffle()  {
        // calling the shuffling
        let copy = this.cardShuffler.shuffle(privateCards);
        // slice(0) is a simple way to do array cloning
        privateCards = copy.slice(0);
    }

    get cards() : Card[]  {
        return privateCards;
    }

    // dealCard is implemented as iterator though the usage of Generator in JavaScript ES6
    // see the code above
    dealCard() : Card {
        return cardSequence.next().value;
    }

}

export default CardDeck;

and in the file cardShuffler.ts

/* CardShuffler.ts */
import {Card, SuiteType} from './card';

// cardShuffler is the contract the define shuffling array of cards

export interface cardShuffler {
    shuffle(inputCards: Card[]) : Card[];
}


// this is a simple implementation of a shuffling algorithm
export class simpleCardShuffler implements cardShuffler {
    shuffle(inputCards: Card[]) : Card[] {
        // we implement a simple shuffling algorithm
        var copy: Card[] = [], n : number = inputCards.length, i : number;
        while(n) {
            i = Math.floor(Math.random() * n--);
            copy.push(inputCards.splice(i, 1)[0]);
        }
        return copy.slice(0);
    }
}


// in the future, and for more requirements we can implement different shuffling logic here, that can be injected into the CardDeck

and finally in the file: CardDeckFactory.ts

/* CardDeckFactory *** */
import CardDeck from './cardDeck';
import {Card, SuiteType} from './card';
import {simpleCardShuffler, cardShuffler} from './cardShuffler';

let buildStandardDeck = function() : Card[]{
    let cards = new Array<Card>();

    for (let suite in SuiteType) {
        if (!isNaN(parseInt(suite))) {
            let suiteAsNumber: number = parseInt(suite);
            for (let rank = 1; rank < 14; rank++) {
                cards.push(new Card(rank, suiteAsNumber));
            }
        }
    }
    return cards;
};

// this is the CardDeck Factory
// it is responsible of building CardDeck with the proper dependencies

export class CardDeckFactory {
    // StandardDeck is a CardDeck with the standard playing cards (52), and the simpleCardShuffler
    public static CreateStandardDeck() : CardDeck {

        return new CardDeck(buildStandardDeck(), new simpleCardShuffler());
    }
}

============================= And for the unit test:

CardDeck-test.ts

/* CardDeck-test.ts */
import { suite, test } from "mocha-typescript";
import {expect} from 'chai';
import * as mocha from 'mocha';
import * as Sinon from 'sinon';

import CardDeck from '../src/model/cardDeck';
import {Card, SuiteType}  from '../src/model/card';
import {cardShuffler, simpleCardShuffler} from '../src/model/cardShuffler';



describe ("CardDeck Tests", function() {

    let testCards: Card[] = [
        new Card(2, SuiteType.Clubs), 
        new Card(11, SuiteType.Hearts), 
        new Card(5, SuiteType.Diamonds),
        new Card(8, SuiteType.Spades),
        new Card(10, SuiteType.Diamonds)
        ];
    let proposedLength : number = testCards.length;


    @suite("When shuffle")
    class Basic {
        subject: CardDeck;
        mockCardShuffler : Sinon.SinonMock;
        before() {
            /* We create a mock for the cardShuffler, and pass the mock to the cardDeck
             If you are used to mock framework in .NET, Java, then you will find
             this mocking unusual.
             Sinon mocking is different that .NET , Java common mocking frameworks.
             As you see we create a concrete object of simpleCardShuffler.
             But even with this, we mock its method "shuffle", so when we call that method, 
             it is not simpleCardShuffler's method that is going to run, but it is our mock. */
            let shuffler = new simpleCardShuffler();
            this.mockCardShuffler = Sinon.mock(shuffler);
            // As you see here , we mock the method shuffle
            this.mockCardShuffler.expects("shuffle").returns([]);
            this.subject =  new CardDeck(testCards, shuffler);
        }


        @test("should call the shuffler passed from constructor")
        assert_shuffle_result_length() {
            // make sure that before shuffle we have all test cards (5)
            expect(this.subject.cards.length).to.equals(proposedLength);
            // Now this call the passed parameter, but as we mocked the method, then it is our mock that will be called.
            this.subject.shuffle();
            expect(this.subject.cards.length).to.equals(0)
            this.mockCardShuffler.verify();  // verify that we got the call 
            this.mockCardShuffler.restore(); // restore everything back and the class simpleCardShuffler will be back as it used to be
        }

    }

    describe ("When deal a card", function() {

        @suite("and when there are cards in the deck")
        class NextCardWithCards {

            subject: CardDeck;

            before() {
                this.subject =  new CardDeck(testCards, null);
                proposedLength = testCards.length;
            }
            @test("should get the first card from the deck") 
            assert_nextCad() {
                let card = this.subject.dealCard();
                expect(card.rank).to.equal(2);
                expect(card.suiteType).to.equal(SuiteType.Clubs);
            }


            @test("and the deck should be reduced by one card") 
            assert_cardRemoved() {
                expect(this.subject.cards.length).to.equal(proposedLength);
                let card = this.subject.dealCard();
                expect(this.subject.cards.length).to.equal(proposedLength - 1);
            }
        }

        @suite("and when there are no more cards in the deck")
        class NextCardWithoutCards {

            subject: CardDeck;

            before() {
                this.subject =  new CardDeck(testCards, null);
            }
            @test("should get undefined") 
            assert_nextCad() {
                var n = testCards.length;
                let localcard : Card;
                while(n--) {
                    localcard = this.subject.dealCard();
                    expect(localcard).to.be.not.null;
                }
                localcard = this.subject.dealCard();
                expect(localcard).to.be.undefined;
            }
        }
    });
});

and for the card test

/* Card-test.ts */

import {suite, test} from 'mocha-typescript';
import * as mocha from 'mocha';
import {Card, PictureCardType, SuiteType} from '../src/model/card';
import {expect} from 'chai';
describe("Card tests", function() {
    @suite("When create a new Card")
    class Basic {
        @test("can pass a numeric value as rank property")
        assert_rank_assignment() {
            var subject: Card = new Card(10, SuiteType.Clubs);

            expect(subject.rank).to.equal(10);
            expect(subject.suiteType).to.equal(SuiteType.Clubs);
        }   

        @test("or passing a picture type as rank property")
        assert_rank_assignment_with_picture_parameter() {
            var subject: Card = new Card(PictureCardType.Jack, SuiteType.Hearts);
            expect(subject.rank).to.equal(11);
            expect(subject.suiteType).to.equal(SuiteType.Hearts);
        }

        @test("should fail when rank is outside 1-13 range")
        assert_fail() {
            expect(() => {
                var subject: Card = new Card(14, SuiteType.Diamonds);
            }).to.throw("rank outside the range");
        }
    }

    @suite("When dealing with picture cards")
    class RankType{
        @test("should be able to switch between the Jack card and its equivalant numeric 11")
        assert_jack() {
            var subject: Card = new Card(PictureCardType.Jack, SuiteType.Diamonds);
            expect(subject.rank).to.equal(11);
            expect(subject.rank).to.equal(PictureCardType.Jack);
        }
    }


            //and dealing with a picture card"
        @suite("When convert a JavaScript Object")
        class JSObjectTest {
            obj: any;
            before() {
                var subject : Card =  new Card(PictureCardType.Jack, SuiteType.Diamonds);
                this.obj = subject.toJSObject();
            }
            @test("should convert suiteType to its string name")
            assert_suitename() {
                expect(this.obj.suite).to.equal('Diamonds');
            }
            @test("should convert rank to its picture name for picture cards")
            assert_jsonPictureCard() {
                expect(this.obj.rank).to.equal('Jack');
            }

            @test("should convert rank to its numeric value numeric cards")
            assert_jsonNumeriCard() {
                var subject : Card =  new Card(5, SuiteType.Diamonds);
                var json: any = subject.toJSObject();
                expect(json.rank).to.equal('5');
            }

        }


});

I won't continue because I think this is enough

So what do you think?

What I did wrong? and how can I do better?

\$\endgroup\$
  • 2
    \$\begingroup\$ I don't know TypeScript. I would have an enum from every rank - two, three ... Start at 0 for both rank and suit (not suite). Use ace as high. As you do calculations this is easier. i / 13 from suite and i % 13 for rank makes for easy generation. There is a standard yates shuffle that is proven to be correct and is very efficient. \$\endgroup\$ – paparazzo Apr 11 '17 at 17:42
  • 5
    \$\begingroup\$ It isn't clear, but do you know you were rejected because of the code? Someone on the interview team could have rejected you before they even gave you the exercise. Or you may have been brought in as a 2nd choice though they knew they were going in their first choice. Having been a part of dozens of tech interviews, the reasons for passing on someone are so varied and (frankly) not always fair. \$\endgroup\$ – Tony Apr 11 '17 at 19:38
  • 12
    \$\begingroup\$ As a purely personal opinion: if it's spread across five files or more and implements an IterableIterator, it doesn't matter what language you are using - it's Java at its worst. Consider it from the perspective of someone who would have to maintain this! How many WTFs per minute does it generate? \$\endgroup\$ – Akshat Mahajan Apr 12 '17 at 1:59
  • 2
    \$\begingroup\$ "a test code": Not proper in English. See english.stackexchange.com/a/20456/97308. \$\endgroup\$ – jpmc26 Apr 12 '17 at 19:03
  • 1
    \$\begingroup\$ The most obvious thing is that you misspelled "suit" and made it "suite". \$\endgroup\$ – Monty Harder Apr 13 '17 at 18:32
26
\$\begingroup\$

While not strictly required, we value usage instructions, nicely-modeled data, automated tests, and thoughtful consideration of architectural decisions and simplicity-vs-completeness trade-offs.

Given this clause, a few things stand out from your code:

  • TypeScript - TypeScript is overkill for this problem. TypeScript is a fine language but not all projects need TypeScript or can even integrate TypeScript in a sane manner. For instance, if you're thrown into a WordPress project that only needs bits and pieces of jQuery, do you really need TypeScript? Your language selection also reflects how you choose tech for a project. Just because you're awesome at TypeScript doesn't mean everyone else can do it. Consider learning curves, ramp time, maintenance. Your code today is someone else's problem tomorrow.

  • Classes, interfaces, patterns - Also overkill. Your deck could simply be an array of objects with suit and value. Your operations could simply be functions that simply accept input and return outputs. New features simply mean new functions. No fancy patterns, factories, interfaces, and all that shiny stuff.

  • Your choice of unit test setup - When you use classes, you tend to have hidden state, which requires accessors, which leads to overcomplicated pre-test setups to get the object state right, which then leads to choosing bloated test runners (*cough* jasmine *cough*) and boilerplate-y test specs (*cough* selenium *cough*). If you just use simple, open data structures and avoid mutation where possible, you won't need to hide properties nor use side-effect-y functions, making everything just a matter of deep comparing between expected and actual data structures.

  • Lack of documentation - Code comments are fine, but there is a structured way to document JS. See JSDoc. In most cases, you only need to know how to write the comment (it starts with /**), @param and @return.

  • Complexity - Just because the instructions said to use classes doesn't mean you should. In the real world, you need the skill to defy instructions in search for a simpler solution, be able to push back on unreasonable requirements, and to ship on time. Many projects don't ship on time because of over-engineered code that ended up causing massive tech debt or overhead to develop or hassle in maintenance.

If I were to build this, it would be one module with 3 functions and an simple QUnit test. Since TypeScript is simply a superset of ES, the following is perfectly valid:

Code

const values = [1,2,3,4,5,6,7,8,9,10,11,12,13];
const suits = ['HEARTS', 'CLUBS', 'SPADES', 'DIAMONDS'];

/**
 * Builds a deck, an array of { suite, value }.
 * @return Array The new deck.
 */
export function createDeck(){
  const values.reduce((c, v) => ([ ...c, ...suits.map(s => ({ value: v, suit: s })) ]), []);
}

/**
 * Shuffles a deck.
 * @param Array deck The deck to shuffle.
 * @return Array The shuffled deck.
 */
export function shuffleDeck(deck){
  // Shuffle logic. Make sure you aren't mutating deck.
  return shuffledDeck;
}

/**
 * Chooses a card from the top of the deck
 * @param Array deck The deck to draw from.
 * @return Object An object that contains the next card and the changed deck
 */
export function dealOneCard(deck){
  if(!deck.length) throw new Error('Deck is empty');
  return { card: deck[0], deck: deck.slice(1) };
}

Test:

import { module, test } from 'qunit';
import { createDeck, shuffleDeck, dealOneCard } from './deck.js';

module('deck');

test('creation', assert => {
  const deck = createDeck();
  // assert if deck is an array
  // assert if it has all the values
});

test('shuffling', assert => {
  const deck = createDeck();
  const newDeck = shuffleDeck(deck);
  // assert if new deck is an array
  // assert if new deck still has all cards
  // assert if new deck isn't in the same order
});

test('dealing a card', assert => {
  const deck = createDeck();
  const state = dealOneCard(deck);
  // check if state.card is a card
  // check if state.deck has one less card
});

While not a requirement today, a likely future enhancement is the need to deal all the cards in a deck.

I would not implement this by the way. Dealing all cards is simply dealing a card until the returned deck becomes empty. This is more like business requirement rather than a coding requirement. Think of it as something required for a certain card game but not all card games. Let the consumer of this API handle that.

\$\endgroup\$
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Mathieu Guindon Apr 13 '17 at 20:33
  • \$\begingroup\$ Honest question, is your first counterpoint (using Typescript) not irrelevant, since the OP was simply asked to develop a piece of business logic, with no frame of reference about where this was going to be implemented? OP chose a frontend language because it's a frontend position, and he could've chosen an easier frontend language; but the interviewer doesn't know why he picked Typescript. The problem definition lists the valued criteria, and the language choice is not listed there, so it seems irrelevant to the interviewer (within reason, e.g. Malbolge would simply be silly) \$\endgroup\$ – Flater Oct 19 '17 at 13:39
39
\$\begingroup\$

No one but the folks who reviewed your code for the company can say with certainty why you were rejected.

It also depends on the position you're applying for. If you're applying for an entry-level role or a guru role, the expectations are drastically different.

As a hiring manager, a couple things jumped out at me (from most to least relevant to me, a potential hiring manager):

  1. You didn't follow directions.

    While not a requirement today, a likely future enhancement is the need to deal all the cards in a deck.

    You went rogue and spent time building a thing you were explicitly told was not a requirement.

    This one is controversial. Some hiring managers will think, "Hey, he went above and beyond." Others will think, "I just asked you to build and shuffle a deck. Is this guy going to constantly try and adjust the requirements on the fly?"

    I see both sides, but would err on the side of follow the directions you're given. Possibly include some comments describing how you would deal the cards and mentioning that you think it would be fairly trivial to add, so you would have talked to your lead/manager about adding it from the get go. Or implement it with a comment that you would have asked before going outside the spec, but assume the manager would have given their ok, so you built it.

  2. Your shuffle test doesn't actually confirm that the deck was shuffled, just that it still exists at the same size. I want to see that the deck isn't in the same order, and that when I shuffle the original deck again, I get a new, third order. I want the UT to make sure the shuffle is at least pseudo-random.

  3. The very first thing I noticed was your second line of code in Cards.ts. Cards do not have suites, they have suits. Some hiring managers might have immediately stopped looking. Misspelling variables is a maintenance nightmare.

  4. Some of your comments have typos, are missing words, or use the wrong word ("help us chose" should be "help us choose")

    Another nitpick, however, this is a job interview. This is your best foot forward. If you're moving to fast and missing details during the interview stage, why wouldn't you make more mistakes once we hire you?

  5. The first time you use .slice(0), you have no comment, but the second time, you do. I suspect you wrote the second instance first along with the comment, but the first instance should have had the comment (if not each instance).

I liked that your CardDeckFactory was extensible to not just a hard coded standard 52 card deck, but could work with Pincochle, Eucher, or other non-standard decks.

I liked that you had the capacity for different shuffle algorithms.

Your code was generally clean, clear and easy to follow.

I like a number of your comments, particularly the longer, more descriptive ones. As someone unfamiliar with TypeScript, I appreciated the comment in the Unit Test about mocking. I also liked your comment about using a Generator. I hadn't seen that before, so it indicates to me that you have a pretty decent depth of understanding.

Others may have more technical reviews.


For what it's worth, if HR handed me the question and answer, I would give my thumbs up for a follow-on interview. In that follow-on, I would ask some questions around issue #1 to better understand why you went off spec.


Edited to add:

Every interviewer is going to key off of different things. Joseph The Dreamer's answer is likely correct in that you went too complex (though I think his answer likely goes too simple to the point of, again, disregarding the directions).

A good thing to keep in mind, especially if this is prior to a phone interview, the person reviewing this thing probably reviews dozens of them daily. Keeping it short, sweet and to the point is probably a good thing. There may be folks who see a couple hundred lines of code and would immediately reject you.

\$\endgroup\$
  • 2
    \$\begingroup\$ I think the capacity for different shuffle algorithms is actually a downside. There are a few different algorithms for correctly shuffling a deck, but only one mathematical standard for judging them to be correct. Include one algorithm for a mathematically correct shuffle and be done. \$\endgroup\$ – Lee Daniel Crocker Apr 11 '17 at 19:50
  • 8
    \$\begingroup\$ "If you're moving to fast and missing details" I'll just assume that was intentional... \$\endgroup\$ – Jeffrey Bosboom Apr 11 '17 at 21:51
  • 3
    \$\begingroup\$ @JeffreyBosboom yes... intentional... :D oops \$\endgroup\$ – Chris G Apr 11 '17 at 22:25
  • 2
    \$\begingroup\$ As a programmer myself: "though I think his answer likely goes too simple" -> Keeping things simple is a skill itself. I've build super fancy ready-for-everything solutions, but the better I got, the simpeler I made my code. I prepared my code (thát is very important), so that it could be easily expanded, but I didn't actually make it. Saved a lot of precious time. This is why I would prefer Joseph's code. It's understandable in a glance. \$\endgroup\$ – Martijn Apr 12 '17 at 8:03
  • 1
    \$\begingroup\$ @Martijn my only issue with it is that it disregards the interviewer's request that you "write a simple class". In the context of an interview question, you could describe your thought process in comments or even provide both your simple solution and their simple solution using a class. \$\endgroup\$ – Chris G Apr 12 '17 at 15:08
27
\$\begingroup\$

This is mostly how I would solve it (in Python):

In [2]: import random

In [3]: class Deck:
   ...:     def __init__(self):
   ...:         self._cards = list(range(52))
   ...:     def shuffle(self):
   ...:         random.shuffle(self._cards)
   ...:     def deal(self):
   ...:         return self._cards.pop()

The data structure is as simple as it gets, the code is almost obviously correct by reading it, and deal_all is a two liner: while self._cards: yield self.deal()

Things like the card picture have nothing to do with the core logic and you don't need to keep them around for every Card in every deck anyway.

\$\endgroup\$
  • 5
    \$\begingroup\$ Why python instead of typescript? \$\endgroup\$ – Stephen Rauch Apr 11 '17 at 17:42
  • 3
    \$\begingroup\$ Because I know Python and know how to solve this particular problem easily with it. The answer shouldn't differ much in other languages though. \$\endgroup\$ – Zah Apr 11 '17 at 17:45
  • 9
    \$\begingroup\$ If I were evaluating this submission from a candidate, I'd reject as incomplete, for failing to implement suits and ranks. \$\endgroup\$ – 200_success Apr 11 '17 at 18:08
  • 15
    \$\begingroup\$ @200_success That was neither in the question nor is it clear how to best do it given just the statement. What is certainly clear, is that a way to represent a single card for this problem (also if you were to implement suits and ranks and pictures and whatnot) is an integer index (from where e.g. list(itertools.product('CSHD',['A', *range(1,11), *'JQK']))[index]), not a microservice. \$\endgroup\$ – Zah Apr 11 '17 at 18:23
  • 6
    \$\begingroup\$ Suits and ranks are trivial here: (n & 0x03), (n >> 2). You could make them functions if you had to, but would probably be faster to inline them. Also, defined this way, just comparing the numbers with < and > ends up comparing ranks without having to break them out. \$\endgroup\$ – Lee Daniel Crocker Apr 11 '17 at 20:00
11
\$\begingroup\$

I sympathise :) I'm not sure if my answer will actually help much as it's either a rant or a list of hard work to do but hopefully it informs.

Interview tests are hopelessly flawed experiments

When we give someone a test in an interview, what are we testing? If it's something along the lines of "this person is a good programmer", and we're testing a pool of candidates, then we're testing a hypothesis and it's an experiment.

Applying quality control to the experiment itself, an interview test would look like this:

  • the test question is selected randomly from a large pool, for all candidates
  • observation of the candidates is anonymous (i.e. commits or something)
  • assessment of the results is also carried out anonymously
  • when all candidates have been tested and assessed, test results are correlated back to their applications
  • all of this is done without the assessors or hiring managers knowing who is doing what

I think this is just a single (or maybe double) blind experiment - I don't know, I'm not a scientist and so I don't run experiments in interviews!

The most important part of this, for you, is that the problem should be randomly selected and the interviewers shouldn't know about it already. The people that interviewed you have pre-existing knowledge of the problem gained from internal staff and previous candidates. This makes them domain experts in the problem and, as with most experts, this will exacerbate their perception of any knowledge or skill gap between you.

Additionally, in the course of becoming experts in the problem, invariably from seeing other people solve it better than they did themselves, they have an inferiority complex about it and will tend to punish their own mistakes when you replicate them.

And, AND! the interviewers are probably actually nice people and feel bad about what they're doing but get caught up in it! There may also be some crazy weird company culture as well. You don't want to work there broski :)

/rant

There is no spoon

The main characteristic (not problem) of your solution is that you've been thinking about the physical reality of cards. This is a great thing if you're an application developer because you'll develop in terms the customer understands.

Becoming an abstract developer is only something to do if you're really interested, I think. It is a powerful skillset as well. I definitely wouldn't call it a career path.

Getting to grips with abstract representations is hard but you can fast track it by practicing functional programming (say a free tutorial book like SICP) and discrete maths.

In software development, you can practice things like design-by-contract, entity-relational modelling, database normalisation & UML. The constraints of all these practices push very hard towards recognising abstraction and the combinatorial qualities of problems.

Code-first imperative programming allows us to think as we code because it's sequential and we can progressively add to it. None of the techniques above are sequential which is part of why they force us to recognise abstraction - a skill that we can internalise the more we practice.

(Ironically they are testing you with the behaviour that will make it harder to learn the things they think they want you to know.)

Here's a practical analysis of your problem:

  • a deck of cards is the cartesian product of two finite sets
  • a shuffle is a new state that is an ordering on a finite set
  • a draw is to select and remove the first card in an ordered set

So what elements do we need in our solution?

  • a finite set
  • a finite set that can be ordered and mutated

Sounds like an array to me!

What about operations?

  • generate cartesian product
  • randomly order
  • pop

Well, array already has pop so we only actually need two functions (that are clearly going to come up in Google)

Now that we have the functional aspects, we can consider the non-functionals:

  • our two sets, rank and suit, could represent their domain in the code (but seeing as our set implementation is an array, they still need to be arrays)
  • each permutation is actually a card and so should be represented as such
  • permutations are results of a calculation, not state and so should be immutable (i.e. card is an immutable class)
  • the definition of an array is essentially a finite ordered mutable set so it matches our problem and we should probably just leave it as is

In the end, we only have to test a cartesian product and a random ordering. Because neither of these need to be specifically about cards, we can use smaller sets to test them and we've also demonstrated re-useable code.

We've also done most of our work without coding, which is good because I'm lazy and I hate writing tests and debugging :)

EDIT: this guy is the bomb - https://en.wikipedia.org/wiki/Fold_(higher-order_function)

\$\endgroup\$
  • 2
    \$\begingroup\$ hear hear! I worked for a company who had the longest weirdest coding test I ever saw.. They were given a piece of paper with a number of thing to do/fix/add each worth a value.. if you did it and got a good working solution then you probably got max points, if you attempted it and left it worse than it was you could LOSE the points it was worth.. At the time of me leaving I had had the highest score and they had been making me do the code test. why did I leave? a girl came got like -1500 as a score, wrote crying on a piece of paper and they employed her and gave her more $$$ than me \$\endgroup\$ – BugFinder Apr 13 '17 at 11:26
  • 1
    \$\begingroup\$ @Seth: A "double-blind" just means that the researchers don't know who's in the control group. (Obviously, the subjects must never know this). I suppose one way to have a control group would be to have the existing team submit answers as well, and throw them into the same anonymous pot... but it's not exactly a double blind, then: only the reviewers do not know which code is from the control group. \$\endgroup\$ – jpaugh Apr 13 '17 at 17:53
5
\$\begingroup\$

The glaring error here is that you use a weird mix of classes and private globals. What happens when someone uses two CardDeck objects simultaneously?

Also, as others have said, your code is overkill (there is very little point in having a Card class or an injected shuffler, for example), which makes it hard to read. Really, this could be as simple as

class CardDeck {
    cards: String[];

    constructor(cards: String[]) {
        this.cards = cards.slice(0);
    }

    shuffle()  {
        // Fisher-Yates
        for (let i = this.cards.length - 1; i--; i > 0) {
            let j = Math.floor(Math.random() * (i + 1));
            [this.cards[i], this.cards[j]] = [this.cards[j], this.cards[i]];
        }
    }

    deal(): String {
        return cards.pop();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Agree. The funny thing is, I started with something similar to what you wrote, but I thought that there is no way I will submit something that simple to the code exercise. So, I added some "flavors" of what a good programmer should have. \$\endgroup\$ – Sam Smith Apr 17 '17 at 13:50
3
\$\begingroup\$

I haven't done a detailed analysis but I scanned the code. Overall I thought it was actually pretty good - although I'm not familiar with the language in question.

There were two things that concerned me.

  • The first was that I agree with the other reviewers saying it seemed rather complex and over-engineered for a very simple thing. Especially when the brief mentioned "simplicity-vs-completeness".

  • The second and connected was that you implemented your own shuffler. Surely there is an existing one in your language? Failure to use an existing library for something like that is a big red flag.

As a side note: Did you have the opportunity to ask any questions about requirements? I had a 30 minute conversation on a phone interview once about data structures, efficiency, and the advantages and disadvantages of various methods as a result of his off-hand question about representing a deck of cards as we discussed the various advantages and disadvantages of completely different ways of building it. That all started because when he asked me how I'd represent it I said "that would depend on what it is being used for" and went on to explain why that mattered.

\$\endgroup\$
  • \$\begingroup\$ Unfortunately, there was no conversation with the employer. It happened through a recruiter, and I just got the result as rejected. \$\endgroup\$ – Sam Smith Apr 11 '17 at 20:49
  • \$\begingroup\$ @SamSmith You can still build some of that conversation into your response. In other words put in a note like "I'm assuming the requirements are X. Be aware that in other circumstances, for example Y I would instead do Z" \$\endgroup\$ – Tim B Apr 11 '17 at 20:51
  • \$\begingroup\$ Yup I will write more comments in next time, it is done now \$\endgroup\$ – Sam Smith Apr 11 '17 at 21:16
  • \$\begingroup\$ @SamSmith For the most part your commenting was good. You did have one pointless comment (Calling the shuffling) but other than that it was good. I don't recommend going overboard on things. Just an introductory comment showing that you understand there are multiple ways to architect this and explaining why you chose the one you did. What you had at the top of card deck explaining the strategy pattern for shuffling for example was good. \$\endgroup\$ – Tim B Apr 12 '17 at 8:13
  • \$\begingroup\$ Wait... can you give me a list of languages that have a built-in shuffler? I can think of zero, maybe one. \$\endgroup\$ – Jasmine Apr 13 '17 at 20:01
1
\$\begingroup\$

I don't know TypeScript but I am poker player
You should use two - ace for rank
Have ace high as will use it that way for calculations
Can easily get suit and rank from an integer Implement card comparison
Not sure that is a valid shuffle
What do you do when no cards are left
This is a C# version

public enum RankSam : byte { Two = 0, Three = 1, Four = 2, Five = 3, Six = 4, Seven = 5, Eight = 6, Nine = 7, Ten = 8, Jack = 9, Queen = 10, King = 11, Ace = 12 }
public enum SuitSam : byte { Spade = 0, Club = 1, Diamond = 2, Heart = 3 }
public class CardSam : IComparable
{
    public override int GetHashCode()
    {
        return quickCard;
    }
    public int CompareTo(object obj)
    {
        if (obj == null)
        {
            throw new ArgumentNullException();
        }
        if (obj is CardSam)
        {
            CardSam other = (CardSam)obj;
            if (other == null)
            {
                throw new ArgumentNullException();
            }
            return (this.Rank).CompareTo(other.Rank);
        }
        else
        {
            throw new ArgumentNullException();
        }
    }
    public override bool Equals(object obj)
    {
        //this is nice for proving shuffle (and other struff)
        if (obj == null)
        {
            throw new ArgumentNullException();
        }
        if (obj is CardSam)
        {
            CardSam other = (CardSam)obj;
            return (this.quickCard == other.quickCard);
        }
        else
        {
            throw new ArgumentNullException();
        }
    }
    byte quickCard;
    public RankSam Rank { get { return (RankSam)(quickCard % 13); } }
    public SuitSam Suit { get { return (SuitSam)(quickCard / 13); } }
    public override string ToString() { return $"{Rank} {Suit}";}
    public CardSam(byte QuickCard)
    {
        if (QuickCard > 51)
            throw new IndexOutOfRangeException("GT 51");
        quickCard = QuickCard;
    }
}
public class DeckSam
{
    private Random rand = new Random();
    private const byte deckSize = 52;
    private int deckIndex = -1;
    private List<CardSam> deck = new List<CardSam>();
    public IReadOnlyCollection<CardSam> Cards { get { return deck.AsReadOnly(); } } 
    public CardSam Deal1()
    {
        deckIndex++;
        if (deckIndex == deckSize)
        {
            Shuffle();  //ask if they want to shuffle here
            deckIndex = 0;
        }
        return deck[deckIndex];
    }
    public void Shuffle()
    {
        //Fisher Yates in place shuffle - proven to be random
        for(int i = deckSize-1; i > 0; i --)
        {
            int j = rand.Next(i+1); //not inclusive in .NET
            if(i != j)
            {
                CardSam swap = deck[i];
                deck[i] = deck[j];
                deck[j] = swap;
            }
        }
    }
    public DeckSam ()
    {
        for (int i = 0; i < deckSize; i++)
            deck.Add(new CardSam((byte)i));
        Shuffle();
    }
}
\$\endgroup\$
  • 21
    \$\begingroup\$ Sorry, any code that contains Five = 3 instantly fails for me as a readability/maintenance problem. If you're going to use enums, give them non-clashing names. Deuce and Jack are probably OK, but the numbers should be FiveSpot or Rank5 or something. \$\endgroup\$ – Lee Daniel Crocker Apr 11 '17 at 20:27
  • \$\begingroup\$ @LeeDanielCrocker You do know you can cast an enum back to a byte? Starting at 0 saves bits. If it did not start at 0 then return (RankSam)(quickCard % 13); would not work. Use the number for the number seems to be pretty standard in cards. \$\endgroup\$ – paparazzo Apr 11 '17 at 20:42
  • \$\begingroup\$ Yes, and I would not have done that either. Dividing by 13 is expensive. Put the suit in the low bits and the rank in the high bits, so rank is now (n >> 2) and suit is (n & 0x03). This also allows you to compare ranks (the most common operation) without splitting them out at all, just by comparing numbers. \$\endgroup\$ – Lee Daniel Crocker Apr 11 '17 at 20:45
  • \$\begingroup\$ @LeeDanielCrocker I thought bit mashing would be a bit mush here. Thanks for your input. \$\endgroup\$ – paparazzo Apr 11 '17 at 20:49
  • 5
    \$\begingroup\$ And finally, that shuffle is not proper Fisher-Yates. The first random number MUST be 0..51, then 0..50, then 0..49, etc. You start with 0..50. As far as I can tell. that leaves the bottom card in place! \$\endgroup\$ – Lee Daniel Crocker Apr 11 '17 at 20:49
0
\$\begingroup\$

If I were the reviewer, I would have stop at this line:

let privateCards : Card[];

You use a global private varibale to store the cards, each time you will instanciate a new deck, that list is going to change. So you can't have multiple decks? This is a major flaw in the design.

This is an immediate red flag for me. The rest of the code is pretty good actually. I like TypeScript, and you use interface and object oriented design well.

Too bad you don't see how wrong it is to use a global variable in your code.

And use a linter. I put your code in tslint, I got 49 linting errors, that is huge!

ERROR: src/card.ts[15, 9]: if statements must be braced
ERROR: src/card.ts[17, 9]: else statements must be braced
ERROR: src/card.ts[30, 5]: The class method 'toJSObject' must be marked either 'private', 'public', or 'protected'
ERROR: src/card.ts[14, 5]: Declaration of constructor not allowed after declaration of private instance method. Instead, this should come after private instance fields.
ERROR: src/card.ts[30, 5]: Declaration of public instance method not allowed after declaration of private instance method. Instead, this should come after constructors.
ERROR: src/card.ts[30, 19]: Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type, the empty type ('{}'), or suppress this occurrence.
ERROR: src/card.ts[6, 1]: Consecutive blank lines are forbidden
ERROR: src/card.ts[2, 65]: Unnecessary semicolon
ERROR: src/card.ts[4, 51]: Unnecessary semicolon
ERROR: src/card.ts[12, 6]: Missing semicolon
ERROR: src/card.ts[14, 31]: Forbidden reference to reserved keyword: type
ERROR: src/card.ts[2, 1]: File missing JSDoc comment at the top-level: src/card.ts
ERROR: src/card.ts[2, 65]: unnecessary semi-colon
ERROR: src/card.ts[4, 51]: unnecessary semi-colon
ERROR: src/CardDeck.ts[5, 11]: Class name must be in pascal case
ERROR: src/CardDeck.ts[4, 1]: Consecutive blank lines are forbidden
ERROR: src/CardDeck.ts[34, 32]: Shadowed name: 'cardShuffler'
ERROR: src/CardDeck.ts[17, 13]: Identifier 'card' is never reassigned; use 'const' instead of 'let'.
ERROR: src/CardDeck.ts[42, 13]: Identifier 'copy' is never reassigned; use 'const' instead of 'let'.
ERROR: src/CardDeck.ts[15, 5]: Identifier 'getNexCard' is never reassigned; use 'const' instead of 'let'.
ERROR: src/CardDeck.ts[22, 5]: Identifier 'cardSequence' is never reassigned; use 'const' instead of 'let'.
ERROR: src/CardDeck.ts[1, 1]: File missing JSDoc comment at the top-level: src/CardDeck.ts
ERROR: src/CardDeck.ts[15, 19]: Use arrow function instead of function expression
ERROR: src/CardDeckFactory.ts[2, 1]: Import sources within a group must be alphabetized.
ERROR: src/CardDeckFactory.ts[3, 9]: Named imports must be alphabetized.
ERROR: src/CardDeckFactory.ts[10, 17]: Identifier 'suiteAsNumber' is never reassigned; use 'const' instead of 'let'.
ERROR: src/CardDeckFactory.ts[8, 14]: Identifier 'suite' is never reassigned; use 'const' instead of 'let'.
ERROR: src/CardDeckFactory.ts[6, 9]: Identifier 'cards' is never reassigned; use 'const' instead of 'let'.
ERROR: src/CardDeckFactory.ts[5, 5]: Identifier 'buildStandardDeck' is never reassigned; use 'const' instead of 'let'.
ERROR: src/CardDeckFactory.ts[9, 20]: Missing radix parameter
ERROR: src/CardDeckFactory.ts[10, 41]: Missing radix parameter
ERROR: src/CardDeckFactory.ts[11, 43]: Forbidden ++ operator
ERROR: src/CardDeckFactory.ts[22, 1]: A stateless class was found. This indicates a failure in the object model: CardDeckFactory
ERROR: src/CardDeckFactory.ts[1, 1]: File missing JSDoc comment at the top-level: src/CardDeckFactory.ts
ERROR: src/CardDeckFactory.ts[8, 5]: Do not use the 'for in' statement: 'for (let suite in SuiteType)'. If this is an object, use 'Object.keys' instead. If this is an array use a standard 'for' loop instead.
ERROR: src/CardDeckFactory.ts[5, 25]: Use arrow function instead of function expression
ERROR: src/CardDeckFactory.ts[6, 17]: Replace Array constructor with an array literal: new Array<Card>()
ERROR: src/cardShuffler.ts[5, 18]: Class name must be in pascal case
ERROR: src/cardShuffler.ts[11, 14]: Class name must be in pascal case
ERROR: src/cardShuffler.ts[12, 5]: The class method 'shuffle' must be marked either 'private', 'public', or 'protected'
ERROR: src/cardShuffler.ts[9, 1]: Consecutive blank lines are forbidden
ERROR: src/cardShuffler.ts[23, 1]: Consecutive blank lines are forbidden
ERROR: src/cardShuffler.ts[14, 9]: Forbidden 'var' keyword, use 'let' or 'const' instead
ERROR: src/cardShuffler.ts[14, 9]: Multiple variable declarations in the same statement are forbidden
ERROR: src/cardShuffler.ts[14, 13]: Identifier 'copy' is never reassigned; use 'const' instead of 'var'.
ERROR: src/cardShuffler.ts[15, 14]: missing whitespace
ERROR: src/cardShuffler.ts[16, 28]: Math.random produces insecure random numbers. Use crypto.randomBytes() or window.crypto.getRandomValues() instead
ERROR: src/cardShuffler.ts[16, 44]: Forbidden -- operator
ERROR: src/cardShuffler.ts[1, 1]: File missing JSDoc comment at the top-level: src/cardShuffler.ts
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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