Code Critique Request

Review of my class

  • (2 Pages)
  • +
  • 1
  • 2

27 Replies - 972 Views - Last Post: 07 April 2010 - 11:03 AM Rate Topic: -----

#1 MentalFloss  Icon User is offline

  • "ADDICTED"[2:5]
  • member icon

Reputation: 526
  • View blog
  • Posts: 1,397
  • Joined: 02-September 09

Code Critique Request

Posted 31 March 2010 - 02:24 PM

Hello.
I'm creating a blackjack game for the hell of it (Another topic about War card game sparked my interest in trying to make a blackjack game).

Anyway, I have a class written that I'd like to get some feedback on. I don't want to give much detail about it because one thing I want is for you guys to tell me the complex areas that are not easily understandable and such.

So, this is a representation of a blackjack hand. Please comment and critique on it if you don't mind.

/// <summary>
/// Represents a collection of playing cards that a player is holding.
/// </summary>
/*
 *  Public Interface:
 *      Properties
 *          Cards               : IEnumerable collection of cards
 *          IsSoftened          : Boolean attribute that when any ace has been represented as one is true
 *          IsBusted            : Boolean attribute that when hand total exceeds bust amount is true
 *          Total               : Integer attribute set to total value of all cards in hand.
 *      Methods
 *          AddCard()           : Method that allows adding a card to the collection of cards in hand.
 *          Refresh()           : Re-calculation of hand's current state in regards to IsSoftened, IsBusted, and Total.
 *          Fold()              : Empties collection of cards and sets IsSoftened and IsBusted to false, and Total to zero.
 */
class BlackjackHand
{
    // Internal collection of cards for this hand.
    private IList<Card> cards = null;

    /// <summary>
    /// Collection of cards in this hand.
    /// </summary>
    public IEnumerable<Card> Cards
    { 
        get 
        {
            Initialize();
            return cards; 
        } 
    }


    /// <summary>
    /// Whether or not this hand is softened by an Ace represented as one instead of eleven.
    /// </summary>
    public bool IsSoftened
    { get; private set; }

    /// <summary>
    /// Whether or not we have gone over bust limit.
    /// </summary>
    public bool IsBusted
    { get; private set; }


    /// <summary>
    /// Represents the total of the hand by face values.
    /// </summary>
    public int Total
    { get; private set; }


    public BlackjackHand()
    {
        Initialize();
    }

    /// <summary>
    /// Singular location to create a new instance of cards. Subsequent runs will essentially be ignored.
    /// </summary>
    private void Initialize()
    {
        if (cards == null)
            cards = new List<Card>();
    }

    /// <summary>
    /// Adds a card to this hand and forces refresh of hand's state.
    /// </summary>
    /// <param name="card">the card to be added.</param>
    public void AddCard(Card card)
    {
        Initialize();
        cards.Add(card);
        Refresh();
    }

    /// <summary>
    /// Re-establishes settings of exposed properties in hand.
    /// Can be called manually, or is called by adding a new card to hand.
    /// </summary>
    public void Refresh()
    {
        // First, get total which also sets IsSoftened.
        Total = CalculateTotal();
        // Next, find out if it's busted yet.
        IsBusted = (Total > BlackjackRules.Bust);
    }

    /// <summary>
    /// Remove all cards from hand and update IsSoftened, IsBusted, and Total to starting values.
    /// </summary>
    public void Fold()
    {
        Initialize();

        cards.Clear();
        IsSoftened = false;
        IsBusted = false;
        Total = 0;
    }

    /// <summary>
    /// Calculates the total of all cards in hand.
    /// Side Effects:
    ///     If aces required softening, sets IsSoftened to true.
    /// </summary>
    private int CalculateTotal()
    {
        Initialize();

        // Method's return variable:
        int total = 0;

        // retrieve total number of aces in this hand:
        int aceCount = cards.Count(card => card.Face == Faces.Ace);

        // Excluding aces, add all face values in hand to total.
        foreach (Card card in cards)
        {
            if (card.Face == Faces.Ace)
                continue;

            total += card.FaceValue;
        }

        // Based on current total, identify how many soft aces are required.
        int softAceCount = GetSoftAceCount(total, aceCount);
        // Hard aces are the left over aces that have not been softened.
        int hardAceCount = aceCount - softAceCount;

        // There's absolutely nothing we can do to manipulate the total any further so add hard aces and soft aces to total:
        total += (hardAceCount * BlackjackRules.HardAce) + softAceCount;

        // If we had to soften an ace, this hand is considered softened.
        IsSoftened = (softAceCount > 0);

        // Preserve calculated total to a non-calculated property.
        Total = total;

        // Finally, return the total
        return total;
    }

    /// <summary>
    /// Based on the total, identify how many aces must be softened for this hand.
    /// </summary>
    private int GetSoftAceCount(int total, int aces)
    {
        // Method's return variable:
        int totalSoftAces = 0;

        // If total is already greater than 21 then all aces need to be softened.
        if (total > BlackjackRules.Bust)
            totalSoftAces = aces;
        // Otherwise, find out what it takes to process all aces while staying as below 21 as possible.
        else
        {
            for (int acesLeft = aces; acesLeft >= aces; acesLeft--)
            {
                // Would treating current ace as hard bust us?
                if ((total + BlackjackRules.HardAce) > BlackjackRules.Bust)
                {
                    // We cannot process any more aces so soften remaining aces and exit loop:
                    totalSoftAces += acesLeft;
                    break;
                }
                // Add a hard ace to total (which is the hand's total)
                else
                {
                    total += BlackjackRules.HardAce;
                }
            }
        }

        // At this point, we know the total soft aces required even if it busts us in the process.
        return totalSoftAces;
    }
}



Thanks.

Is This A Good Question/Topic? 0
  • +

Replies To: Code Critique Request

#2 FlashM  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 382
  • View blog
  • Posts: 1,195
  • Joined: 03-December 09

Re: Code Critique Request

Posted 31 March 2010 - 02:51 PM

I know the game only by its name, so it's really hard to comment the business logic of this class and whether you could write any method better than it is... I think the code is good and understandable, there is just one thing that bothers me a bit and that is the call to Initialize method in many different places in your class... The class should be initialized only once... You should try to remove that line of code from your methods and properties or if you really need to do that, I'd choose some other name for this method...
Was This Post Helpful? 1
  • +
  • -

#3 MentalFloss  Icon User is offline

  • "ADDICTED"[2:5]
  • member icon

Reputation: 526
  • View blog
  • Posts: 1,397
  • Joined: 02-September 09

Re: Code Critique Request

Posted 31 March 2010 - 03:08 PM

View PostFlashM, on 31 March 2010 - 01:51 PM, said:

I think the code is good and understandable, there is just one thing that bothers me a bit and that is the call to Initialize method in many different places in your class... The class should be initialized only once... You should try to remove that line of code from your methods and properties or if you really need to do that, I'd choose some other name for this method...


I was thinking the same thing. It's the first time I'm trying something like this but I figured I can either do that or do a bunch of

if (cards == null)
    cards = new List<Card>();



Instead, I opted to put that into the Initialize method which for the most part when called will do nothing. It doesn't require me to put flow-control statements in anywhere so I can just make the simple call to Initialize() and not worry about null instances.

Now, to the point of renaming it, I can get behind that idea. Any suggestions? Maybe EnsureHandExists()? or VerifyInstnaces()? I dunno.
Was This Post Helpful? 0
  • +
  • -

#4 FlashM  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 382
  • View blog
  • Posts: 1,195
  • Joined: 03-December 09

Re: Code Critique Request

Posted 31 March 2010 - 03:13 PM

create a new instance of cards variable only in your class constructor... If by some wierd reason it happens that your class has been initialized and your cards list is null, that means something has gone terribly wrong :-)
Was This Post Helpful? 1
  • +
  • -

#5 MentalFloss  Icon User is offline

  • "ADDICTED"[2:5]
  • member icon

Reputation: 526
  • View blog
  • Posts: 1,397
  • Joined: 02-September 09

Re: Code Critique Request

Posted 31 March 2010 - 03:53 PM

Quote

create a new instance of cards variable only in your class constructor... If by some wierd reason it happens that your class has been initialized and your cards list is null, that means something has gone terribly wrong :-)


Normally how I do it but I'm questioning whether or not that's acceptable practice. Yes, common opinion is what you're stating but just because it's popular doesn't mean it's correct/best. Here's my reasoning behind this in slightly more detail:

1. Evolution of class would be resistant to accidental removal of instantiation (not likely to happen, but possible) whereas with the method called everywhere, its removal will cause errors. You have to deliberately want Initialize() out of there.

2. I don't have to check for null everywhere before trying to use what I believe to be a valid instance. I can let just that method do it for me.

3. Should the class need enhancements with perhaps another collection, it too can go into Initialize() which will have the same benefits detailed in 1.

Reasons against:

1. Not really common or normal. Kinda makes you go WTF?

2. Yeah, why would the collection ever be null?

3. Method name is certainly debatable


Overall, I am going to see where this pattern leads me. I like it from the aspect of added protection in project infancy stages. I like the resiliency that it gives to changes that may come up (It's entirely possible to wipe the constructor out in this class and not know anything is bad until you try to use the collection).

You're right in debating its purpose though. I think to truly identify whether or not the idea is worth doing is to just keep trying it on this project and seeing what happens.

Thanks for the insight by the way.
Was This Post Helpful? 1
  • +
  • -

#6 FlashM  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 382
  • View blog
  • Posts: 1,195
  • Joined: 03-December 09

Re: Code Critique Request

Posted 31 March 2010 - 04:02 PM

OK, let take a look at another example... Let's say that your cards instance would be for some unknown reason set to null. When you would call your Cards property to retrieve all the cards, this property method would call the Initialize method which would instantiate a new empty cards list. So fine so good... You got your cards list and no exception has been thrown... But then I would be really surprized that my cards list is suddenly empty (WTF went wrong??? Where are my cards???) And even worse... I wouldn't even know that my cards list has ever been accidentally set to null by some strange reason...

But I certainly do understand what you are trying to achieve and in some cases that wouldn't even be that wrong...
Was This Post Helpful? 1
  • +
  • -

#7 MentalFloss  Icon User is offline

  • "ADDICTED"[2:5]
  • member icon

Reputation: 526
  • View blog
  • Posts: 1,397
  • Joined: 02-September 09

Re: Code Critique Request

Posted 31 March 2010 - 04:11 PM

View PostFlashM, on 31 March 2010 - 03:02 PM, said:

OK, let take a look at another example... Let's say that your cards instance would be for some unknown reason set to null. When you would call your Cards property to retrieve all the cards, this property method would call the Initialize method which would instantiate a new empty cards list. So fine so good... You got your cards list and no exception has been thrown... But then I would be really surprized that my cards list is suddenly empty (WTF went wrong??? Where are my cards???) And even worse... I wouldn't even know that my cards list has ever been accidentally set to null by some strange reason...


True enough. However, what happens when I have no cards in the hand? The dealer can make a check and if the list is actually empty, deal the 2 cards required. So far, no exceptions - no try/catches. It's not the responsibility of the Hand object to ensure that the items in its collection are valid - it's the job of the dealer or rather whatever is consuming the Hand object.

For now, I will keep that segment as is. With that untested technique, I intend to do it everywhere so I can see where the problems behind it pop up.

Anyway, besides the wonky nature of Initialize() anything else you see that is of interest for analysis?
Was This Post Helpful? 1
  • +
  • -

#8 FlashM  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 382
  • View blog
  • Posts: 1,195
  • Joined: 03-December 09

Re: Code Critique Request

Posted 31 March 2010 - 04:31 PM

I completely agree with you if you are looking at this problem from human perspective. But every application needs to have some application logic and those rules are going to break down... And in the end you will end up writing some artificial intelligence to handle that kind of situations :-) joke... well, it's definitely cool to try out other approaches... Please do report all the positive and negative sides you will encounter by trying this out. I'll keep my fingers crossed for you :-)
Was This Post Helpful? 0
  • +
  • -

#9 darkrain714  Icon User is offline

  • New D.I.C Head

Reputation: 2
  • View blog
  • Posts: 24
  • Joined: 15-January 09

Re: Code Critique Request

Posted 01 April 2010 - 07:53 AM

I may be mistaken, but if the hand mysteriously gets set to null wouldn't you want the program to break during debugging so you can fix the logic error you've unknowingly inserted?
Was This Post Helpful? 1
  • +
  • -

#10 MentalFloss  Icon User is offline

  • "ADDICTED"[2:5]
  • member icon

Reputation: 526
  • View blog
  • Posts: 1,397
  • Joined: 02-September 09

Re: Code Critique Request

Posted 01 April 2010 - 10:25 AM

Quote

I may be mistaken, but if the hand mysteriously gets set to null wouldn't you want the program to break during debugging so you can fix the logic error you've unknowingly inserted?


The whole idea I'm experimenting with here is to avoid even introducing occasions where it's possible. Everything using the dependent object ensures it's usable first. It's acceptable that it's empty but it's not acceptable that it's null.

So, with this, it prevents me from having to worry about checking for null everywhere else or using try/catches that might access the collection.

Since, this class exposes the collection through an IEnumerable property, it's OK for it to return no results but it's not OK for it to explode on null.

Now, in terms of the collection being empty considered a bad state, that's some other object's responsibility. That's a business rule. The Hand object shouldn't care that there's not two cards in the collection.
Was This Post Helpful? 0
  • +
  • -

#11 darkrain714  Icon User is offline

  • New D.I.C Head

Reputation: 2
  • View blog
  • Posts: 24
  • Joined: 15-January 09

Re: Code Critique Request

Posted 01 April 2010 - 08:20 PM

View PostMentalFloss, on 01 April 2010 - 09:25 AM, said:

Quote

I may be mistaken, but if the hand mysteriously gets set to null wouldn't you want the program to break during debugging so you can fix the logic error you've unknowingly inserted?


The whole idea I'm experimenting with here is to avoid even introducing occasions where it's possible. Everything using the dependent object ensures it's usable first. It's acceptable that it's empty but it's not acceptable that it's null.

So, with this, it prevents me from having to worry about checking for null everywhere else or using try/catches that might access the collection.

Since, this class exposes the collection through an IEnumerable property, it's OK for it to return no results but it's not OK for it to explode on null.

Now, in terms of the collection being empty considered a bad state, that's some other object's responsibility. That's a business rule. The Hand object shouldn't care that there's not two cards in the collection.



but the only reason you would even need the Initialize() in front of every block would be if your hand was set to null (which will not happen on its own) in which case you have a logic error in your program. It would be better to have your program crash in this case so you know you have a logic error, and then debug it so that you can remove that error. What you are doing now does work, but it is terrible coding practice, and a band-aid fix.
Was This Post Helpful? 1
  • +
  • -

#12 MentalFloss  Icon User is offline

  • "ADDICTED"[2:5]
  • member icon

Reputation: 526
  • View blog
  • Posts: 1,397
  • Joined: 02-September 09

Re: Code Critique Request

Posted 01 April 2010 - 11:33 PM

I'm going to go with the flow on this one. You guys are right. This is a bad idea.

Thanks.
Was This Post Helpful? 0
  • +
  • -

#13 FlashM  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 382
  • View blog
  • Posts: 1,195
  • Joined: 03-December 09

Re: Code Critique Request

Posted 02 April 2010 - 01:32 AM

How come you suddenly changed your mind MentalFloss? :-)
Was This Post Helpful? 0
  • +
  • -

#14 MentalFloss  Icon User is offline

  • "ADDICTED"[2:5]
  • member icon

Reputation: 526
  • View blog
  • Posts: 1,397
  • Joined: 02-September 09

Re: Code Critique Request

Posted 02 April 2010 - 09:36 AM

View PostFlashM, on 02 April 2010 - 12:32 AM, said:

How come you suddenly changed your mind MentalFloss? :-)


I asked for code review from several coworkers and they all harped on the same thing. Also, much of the comments in here make perfect sense. It really is a band-aid or circumvention of a bigger problem. I just figured that it would be better to do it that way, but yeah it absolutely would mask a much bigger problem in the long run.

So, thanks guys. For now, with this piece of code, I have what I needed to hear. I'm rewriting the whole concept behind this class anyway and I'll post an update for more code review (if it's not too much trouble) when it's ready.

Again, thanks. :)
Was This Post Helpful? 0
  • +
  • -

#15 darkrain714  Icon User is offline

  • New D.I.C Head

Reputation: 2
  • View blog
  • Posts: 24
  • Joined: 15-January 09

Re: Code Critique Request

Posted 02 April 2010 - 09:59 AM

View PostMentalFloss, on 02 April 2010 - 08:36 AM, said:

View PostFlashM, on 02 April 2010 - 12:32 AM, said:

How come you suddenly changed your mind MentalFloss? :-)


I asked for code review from several coworkers and they all harped on the same thing. Also, much of the comments in here make perfect sense. It really is a band-aid or circumvention of a bigger problem. I just figured that it would be better to do it that way, but yeah it absolutely would mask a much bigger problem in the long run.

So, thanks guys. For now, with this piece of code, I have what I needed to hear. I'm rewriting the whole concept behind this class anyway and I'll post an update for more code review (if it's not too much trouble) when it's ready.

Again, thanks. :)


It never hurts to be creative though :) It's just in this case it could turn out badly :x
Was This Post Helpful? 0
  • +
  • -

  • (2 Pages)
  • +
  • 1
  • 2