Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to write a function called countHand() that takes in an array of Card instances and counts the total value of the cards given.

The requirements are as follows:

  1. The function returns the value of the cards in the hand as an Int. It does not use loops or nested functions.
  2. The card values are as follows:
    • Any Ace preceded by 5 of Diamonds is worth 100 points.
    • Any odd numeric card (3, 5, 7, 9) of any suit's worth the double of its rank value in points when immediately preceded in the hand by any card of the Hearts.

Examples:

  • The hand 3♥, 7♣ has total value of 14.
  • The hand 3♣, 7♥ has total value of 0.

Brevity/using one statement functions/closures/case statements are prefered.

Here is the current solution:

enum Suit {
    case Clubs, Diamonds, Hearts, Spades
}

enum Rank {
    case Jack, Queen, King, Ace
    case Num(Int)
}

struct Card {
    let suit: Suit
    let rank: Rank
}


/*
- Generate an array of tuples of 2 card subsequences by zipping the hand array with itself (minus the first element)
- Map over the array examining each pair for value-contributing combinations in a switch statement
- Sum all resulting values
*/
func countHand(hand: [Card]) -> Int {
    return Array(Zip2(hand, dropFirst(hand))).map{(card1: Card, card2: Card)->Int in
        switch (card1.suit, card1.rank, card2.rank) {
        case (.Hearts, _, .Num(let numRank)) where numRank % 2 == 1:
            return 2 * numRank
        case (.Diamonds, .Num(5), .Ace):
            return 100
        default:
            return 0
        }}.reduce(0, +)
}


countHand([
    Card(suit:.Hearts, rank:.Num(10)),
    Card(suit:.Hearts, rank:.Num(6)),
    Card(suit:.Diamonds, rank:.Num(5)),
    Card(suit:.Clubs, rank:.Ace),
    Card(suit:.Diamonds, rank:.Jack)
    ])// --> prints 110

I am wondering if there is room for improvement of this code, whether it would be brevity/execution speed/memory usage/CPU (extreme cases could be considered i.e a hand with a big number of cards).

share|improve this question

migrated from stackoverflow.com Jul 26 at 19:39

This question came from our site for professional and enthusiast programmers.

    
Please do not remove the code from the question; that will invalidate the answers. –  Jamal Jul 30 at 0:56
    
Only a moderator can do that if there are upvoted answers, and I will not do that as it's disrespectful to the answerers who've spent time to help you. –  Jamal Jul 30 at 0:59
    
The system only allows it if there are no upvoted answers. You would need a very good reason for removing it anyway, also considering that the code you've posted is already licensed under CC BY-SA. –  Jamal Jul 30 at 1:03
    
For what reason would you want to remove this question? –  nhgrif Aug 2 at 13:38

1 Answer 1

First a minor note. It is less efficient to create a temporary Array from the result of Zip2. You can use the function map instead of the method map:

map(Zip2(hand, dropFirst(hand))) { ... }.reduce(0, +)

Second, I believe that having such a complex comment describing what your method does, is a sign that your code can be much more understandable. Usually a good replacement for a comment is a well named function. This is how I would implement your function:

func countHand(hand: [Card]) -> Int {
    func iterateTwoCardsAtATime(hand: [Card], block: (first: Card, second: Card) -> ()) {
        for (first, second) in Zip2(hand, dropFirst(hand)) {
            block(first: first, second: second)
        }
    }

    func scoreForConsecutiveCards(first: Card, second: Card) -> Int {
        switch(first.suit, first.rank, second.rank) {
            case (.Hearts, _, .Num(let numRank)) where numRank % 2 == 1:
                return 2 * numRank
            case (.Diamonds, .Num(5), .Ace):
                return 100
            default:
                return 0
        }
    }

    var sum = 0
    iterateTwoCardsAtATime(hand) { (first, second) in
        sum += scoreForConsecutiveCards(first, second)
    }
    return sum
}

Yes, this is definitely more verbose, but the code is much more clear and doesn't require a comment.

First, a new developer looks at the method and sees that there is a function called iterateTwoCardsAtATime. If they want to verify that it works as expected, they can, but most can assume it works and skip that implementation.

Then they get to scoreForConsecutiveCards. This contains all of the complicated scoring logic which is probably the biggest concern when evaluating this method. It is well contained and distinct from the other logic of the method. The other logic is very utilitarian while this logic is important business logic.

Finally, there is a simple understandable block of code written in English like language to sum up the results of all of the scores.

With modern IDEs with features like code folding, clarity is way more important than brevity.

share|improve this answer
    
The last comment I find particularly appropriate. There's no reason for our code to be short. We can use multiple files and fold our code up. It's important for our code to be readable. And if it's efficiency you're asking for, well that's one thing... but a variable with a 2 letter name is not more efficient than a variable with a 20 letter name. –  nhgrif Aug 2 at 13:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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