5
\$\begingroup\$

I'm learning about OOP and I'm attempting to solve the White Elephant Gift Exchange game. The way I've done it is that I have created a bunch of set variables which store the set of gifts opened, set of gifts not opened yet, set of users. I was wondering if this is a good way of designing the classes and delegating work.

Could someone please comment on how can the design be improved and how could I make it more object-oriented?

Game rules

In its most basic form, the game is as follows: each participant supplies one wrapped gift. The gifts are placed in a central location, and participants determine in what order they will take turns selecting them. The first person opens a wrapped gift and the turn ends. On subsequent turns, each person can open a new present or gets the choice to "steal" another person's unwrapped gift. When a person's gift is stolen, that person can either choose another wrapped gift to open or can steal from another player. The game is over when the last person goes and the first person goes again (but see variations below).

User.java

class User {
    private Gift giftBrought;
    private Gift giftWon;

    public Gift pickOrSteal(Set<Gift> unopnenedGifts, Set<Gift> openedGifts) {}
}

Gift.java

class Gift {
    private boolean isOpened;
    private User owner;
}

WhiteElephant.java

class WhiteElephant {
    private List<User> users;
    private Set<User> turnCompletedUsers;
    private Set<Gift> unopnenedGifts;
    private Map<Gift, User> openedGifts;
    private Set<Gift> unavailableForRoundGifts;

    public void assignRanks() {
        // use knuth shuffle to randomize user array
    }

    public List<Gift> getUnopenedGifts() {}
    public List<Gift> getOpenedGifts() {}

    public boolean isGiftAvailableForStealing(Gift gift) {
        return !unavailableForRoundGifts.contains(gift);
    } 

    public Gift pickGift() {}

    public boolean stealGift(User from, User to, Gift gift) {}

    public void play() {
         assignRanks();

         for(int i = 0; i < users.size(); i++) {
             nextUser = users.get(i);
             unavailableForRoundGifts.clear();

             while(nextUser != null) {
                 User currUser = nextUser;
                 Gift gift = currUser.pickOrSteal(unopenedGifts, openedGifts.keyset());
                 if(isGiftAvailableForStealing(gift)) {
                     if(!gift.isOpened()) {
                         unopenedGifts.remove(gift);
                         openedGifts.add(gift);
                         gift.setIsOpened(true);
                         nextUser = null;
                     } else {
                         nextUser = openedGifts.get(gift);
                         stealGift(nextUser, users.get(i), gift);
                     }
                     unavailableForRoundGifts.add(gift);
                     users.get(i).setGiftWon(gift);
                 }
             }
         }
    }
} 
\$\endgroup\$

migrated from cs.stackexchange.com Mar 15 '15 at 22:19

This question came from our site for students, researchers and practitioners of computer science.

  • 3
    \$\begingroup\$ Hi. Welcome to Code Review! As this was migrated from a different site, I edited the question to fit our format a bit better. Apologies if I misstated anything. After you have a chance to read our Help section, feel free to adjust. \$\endgroup\$ – Brythan Mar 15 '15 at 23:38
1
\$\begingroup\$

Technical issues

  • In your WhiteElephant class, turnCompletedUsers is unused. Is this expected?
  • For the purposes of this site*, I think it'll be better if you can actually fill in the implementation for WhiteElephant.stealGift() and WhiteElephant.pickGift() (which happens to be unused, too).
  • You do not need to make all your methods in WhiteElephant public, probably only play() should remain public and the rest can be private.
  • openedGifts.add(gift) is actually a syntax error*, as openedGifts is a Map. Perhaps you were looking for openGifts.put(gift, currentUser)?
  • stealGift() returns a boolean value, but what does that indicate?

Gameplay

The first person opens a wrapped gift and the turn ends.

Your implementation does not take care of the first player, where the turn ends immediately after opening a gift.

Gift gift = currUser.pickOrSteal(unopenedGifts, openedGifts.keySet());
if (isGiftAvailableForStealing(gift)) {
    ...
}

This two lines seem a bit weird, as I don't think the game rules makes a separate decision concerning whether a gift is available for stealing after a player picks/steals a gift.

An alternative approach

I'll leave this as an exercise for the reader, how about considering adding a new field inside Gift that represents the current holder of the item? In that case, the same isOpened() method will read something like:

public boolean isOpened() {
    return holder != null;
}

The other operations to complete the 'stealing' of gifts, i.e. swapping the players holding the gift, becomes slightly easier, I would think...


* In fact, we only review complete and working code, I am letting these slide because it's not too far a stretch to assume what might be the underlying implementation... but only just.

\$\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.