CODEWITHSUNDEEP
java
user2947058
user2947058

Reputation: 13

Simplifying convenience constructor

I have a working convenience constructor, but I feel like it is way too much code. I'm not sure how to simplify it, but I would appreciate any help!

public Hand(Card c1, Card c2, Card c3, Card c4, Card c5, Card c6) {
    this();
    this.card1 = c1;
    this.card2 = c2;
    this.card3 = c3;
    this.card4 = c4;
    this.card5 = c5;
    this.card6 = c6;
    if (c1 != null && c2 != null && c3 != null && c4 != null && c5 != null && c6 != null) {
        for (int count = 0; count < 6; count++) {
            if (count == 0) {
                cardsInHand.add(c1);
            } else if (count == 1) {
                cardsInHand.add(c2);
            } else if (count == 2) {
                cardsInHand.add(c3);
            } else if (count == 3) {
                cardsInHand.add(c4);
            } else if (count == 4) {
                cardsInHand.add(c5);
            } else if (count == 5) {
                cardsInHand.add(c6);
            }
        }
    }
}

EDIT: Cleaned up the code with a suggestion below. The program still runs with the following code:

public Hand(Card c1, Card c2, Card c3, Card c4, Card c5, Card c6) {
    this();
    this.card1 = c1;
    this.card2 = c2;
    this.card3 = c3;
    this.card4 = c4;
    this.card5 = c5;
    this.card6 = c6;

    cardsInHand.add(c1);
    cardsInHand.add(c2);
    cardsInHand.add(c3);
    cardsInHand.add(c4);
    cardsInHand.add(c5);
    cardsInHand.add(c6);

Upvotes: 0

Views: 561

Answers (2)

vandale
vandale

Reputation: 3650

The for loop is redundant as it will get added in that order so

cardsInHand.add(c1);
cardsInHand.add(c2);
//etc

will do the same thing.

You could also consider using a var args constructor:

public Hand(Card ... cards){
   this.card1=cards[0];
   //etc

Upvotes: 1

Makoto
Makoto

Reputation: 106430

Your for loop is both unnecessary and confusing at the same time. What you're doing is adding each card to the list, and the order it falls into is natural as opposed to forced - unless you explicitly enter a card that isn't the first parameter, then you won't be entering them out of order.

Not that it should matter, really. A hand of cards is more like a skip-list, where insertion order matters little.

Furthermore, since you claim to have a backing list for your cards, your fields mean little, too. Let's get rid of those as well, and just add the values directly into the list.

I'm going to use a varargs constructor - this way, you don't always have to add just five cards to your hand on initialization.

public Hand(Card... cards) {
    for(Card c: cards) {
        if(c != null) { // if you want to be really careful...
            cardsInHand.add(c);
        }
    }
}

Upvotes: 1

Related Questions