Skip to main content
added 3073 characters in body
Source Link
h.j.k.
  • 19.2k
  • 3
  • 36
  • 93

edit further explanation for an enum-approach

Say you have a Rank and Suit enum:

enum Rank {
    ONE,
    TWO,
    // ...
    JACK,
    QUEEN,
    KING,
    ACE; // opt-ing to 'rank' ACE as the highest

    public int getValue() {
        return ordinal() + 1;
    }
}

enum Suit {
    CLUB,
    DIAMOND,
    HEART,
    SPADE;
}

Some benefits of using enums are:

  • Values are Comparable to each other.
  • Efficient use in Maps and Sets via EnumMap and EnumSet classes, when required.
  • built-in toString() representation.
  • built-in index-based lookup for values via ordinal().
  • built-in String-based lookup for values via valueOf(String).
  • safe ==-based comparison.

For your Card class, you can then do something like:

// note: final modifier to indicate this class cannot be subclassed
// in order to preserve the immutable features
public final class Card {
    private final Rank rank;
    private final Suit suit;
    private final String toString;
    private final int hashCode;

    public Card(Rank rank, Suit suit) {
        this.rank = rank;
        this.suit = suit;
        this.toString = getToString(rank, suit);
        // Objects.hash() is from Java 7
        this.hashCode = Objects.hash(rank, suit);
    }

    private static String getToString(Rank rank, Suit suit) {
        // code review comment: will return e.g. "Seven of Hearts"
        return String.format("%s of %ss",
                    capitalizeFirstLetter(rank.toString()),
                    capitalizeFirstLetter(suit.toString()));
    }

    @Override
    public String toString() {
        return toString;
    }

    @Override
    public int hashCode() {
        return hashCode;
    }

    @Override
    public boolean equals(Object o) {
        if (!(o instanceof Card)) {
            return false;
        }
        Card otherCard = (Card) o;
        return otherCard.rank == rank && otherCard.suit == suit;
    }
}

I hope this kind of illustrates that your Value/Rank, Suit and Card classes can have similar functionality with less boilerplate-like template code... The idea behind pre-computing the String representation and hash code first is to avoid repeated computations, should the methods be called frequently. This works especially well for immutable classes like the Card class as shown.

edit further explanation for an enum-approach

Say you have a Rank and Suit enum:

enum Rank {
    ONE,
    TWO,
    // ...
    JACK,
    QUEEN,
    KING,
    ACE; // opt-ing to 'rank' ACE as the highest

    public int getValue() {
        return ordinal() + 1;
    }
}

enum Suit {
    CLUB,
    DIAMOND,
    HEART,
    SPADE;
}

Some benefits of using enums are:

  • Values are Comparable to each other.
  • Efficient use in Maps and Sets via EnumMap and EnumSet classes, when required.
  • built-in toString() representation.
  • built-in index-based lookup for values via ordinal().
  • built-in String-based lookup for values via valueOf(String).
  • safe ==-based comparison.

For your Card class, you can then do something like:

// note: final modifier to indicate this class cannot be subclassed
// in order to preserve the immutable features
public final class Card {
    private final Rank rank;
    private final Suit suit;
    private final String toString;
    private final int hashCode;

    public Card(Rank rank, Suit suit) {
        this.rank = rank;
        this.suit = suit;
        this.toString = getToString(rank, suit);
        // Objects.hash() is from Java 7
        this.hashCode = Objects.hash(rank, suit);
    }

    private static String getToString(Rank rank, Suit suit) {
        // code review comment: will return e.g. "Seven of Hearts"
        return String.format("%s of %ss",
                    capitalizeFirstLetter(rank.toString()),
                    capitalizeFirstLetter(suit.toString()));
    }

    @Override
    public String toString() {
        return toString;
    }

    @Override
    public int hashCode() {
        return hashCode;
    }

    @Override
    public boolean equals(Object o) {
        if (!(o instanceof Card)) {
            return false;
        }
        Card otherCard = (Card) o;
        return otherCard.rank == rank && otherCard.suit == suit;
    }
}

I hope this kind of illustrates that your Value/Rank, Suit and Card classes can have similar functionality with less boilerplate-like template code... The idea behind pre-computing the String representation and hash code first is to avoid repeated computations, should the methods be called frequently. This works especially well for immutable classes like the Card class as shown.

added 3 characters in body
Source Link
h.j.k.
  • 19.2k
  • 3
  • 36
  • 93

For starters, using Java enums will likely save you a lot of the boilerplate-like template code you have for Value and Suit.

Your Card constructors seem to be over-engineered too... what you have are just permutations of ints, Strings and objects arguments... I suggest you pick one and stick with that, preferably just Value and Suit. If your Card objects are meant to be immutable, I'll suggest saving its String representation (used, naturally, in toString()) as an instance field first. In fact, you should probably go a step further and override the equals() and hashCode() methods too to simplify/standardize such usage for the class. This will be helpful if you are using them in most, if not all, Collection classes.

Unfortunately, your implementation for capitalizeFully() doesn't seem too good... First, I'm not sure what it's meant by 'fully' capitalizing a String. I thought you meant converting a String to uppercase. Turns out you are just capitalizing the first letter. So why not call that then?

private static String capitalizeFirstcapitalizeFirstLetter(String input) {
    // ...
}

Unless you are constructing Strings from byte/char arrays, with an optional Charset, you should (almost) never need to instantiate a String by doing new String(). Then, using the toLowerCase() and toUpperCase() methods...

private static String capitalizeFirstcapitalizeFirstLetter(String input) {
    // handle null or "" Strings if required
    return input.substring(0, 1).toUpperCase() + input.substring(1).toLowerCase();
}

If you want to cater for Locale-specific conversions, there are methods for that too.

Last, you need a much better name for your print (?) p() method... what is it supposed to do actually, anyways? The returned object doesn't appear to be used in any way.

For starters, using Java enums will likely save you a lot of the boilerplate-like template code you have for Value and Suit.

Your Card constructors seem to be over-engineered too... what you have are just permutations of ints, Strings and objects arguments... I suggest you pick one and stick with that, preferably just Value and Suit. If your Card objects are meant to be immutable, I'll suggest saving its String representation (used, naturally, in toString()) as an instance field first. In fact, you should probably go a step further and override the equals() and hashCode() methods too to simplify/standardize such usage for the class. This will be helpful if you are using them in most, if not all, Collection classes.

Unfortunately, your implementation for capitalizeFully() doesn't seem too good... First, I'm not sure what it's meant by 'fully' capitalizing a String. I thought you meant converting a String to uppercase. Turns out you are just capitalizing the first letter. So why not call that then?

private static String capitalizeFirst(String input) {
    // ...
}

Unless you are constructing Strings from byte/char arrays, with an optional Charset, you should (almost) never need to instantiate a String by doing new String(). Then, using the toLowerCase() and toUpperCase() methods...

private static String capitalizeFirst(String input) {
    // handle null or "" Strings if required
    return input.substring(0, 1).toUpperCase() + input.substring(1).toLowerCase();
}

If you want to cater for Locale-specific conversions, there are methods for that too.

Last, you need a much better name for your print (?) p() method... what is it supposed to do actually, anyways? The returned object doesn't appear to be used in any way.

For starters, using Java enums will likely save you a lot of the boilerplate-like template code you have for Value and Suit.

Your Card constructors seem to be over-engineered too... what you have are just permutations of ints, Strings and objects arguments... I suggest you pick one and stick with that, preferably just Value and Suit. If your Card objects are meant to be immutable, I'll suggest saving its String representation (used, naturally, in toString()) as an instance field first. In fact, you should probably go a step further and override the equals() and hashCode() methods too to simplify/standardize such usage for the class. This will be helpful if you are using them in most, if not all, Collection classes.

Unfortunately, your implementation for capitalizeFully() doesn't seem too good... First, I'm not sure what it's meant by 'fully' capitalizing a String. I thought you meant converting a String to uppercase. Turns out you are just capitalizing the first letter. So why not call that then?

private static String capitalizeFirstLetter(String input) {
    // ...
}

Unless you are constructing Strings from byte/char arrays, with an optional Charset, you should (almost) never need to instantiate a String by doing new String(). Then, using the toLowerCase() and toUpperCase() methods...

private static String capitalizeFirstLetter(String input) {
    // handle null or "" Strings if required
    return input.substring(0, 1).toUpperCase() + input.substring(1).toLowerCase();
}

If you want to cater for Locale-specific conversions, there are methods for that too.

Last, you need a much better name for your print (?) p() method... what is it supposed to do actually? The returned object doesn't appear to be used in any way.

Source Link
h.j.k.
  • 19.2k
  • 3
  • 36
  • 93

For starters, using Java enums will likely save you a lot of the boilerplate-like template code you have for Value and Suit.

Your Card constructors seem to be over-engineered too... what you have are just permutations of ints, Strings and objects arguments... I suggest you pick one and stick with that, preferably just Value and Suit. If your Card objects are meant to be immutable, I'll suggest saving its String representation (used, naturally, in toString()) as an instance field first. In fact, you should probably go a step further and override the equals() and hashCode() methods too to simplify/standardize such usage for the class. This will be helpful if you are using them in most, if not all, Collection classes.

Unfortunately, your implementation for capitalizeFully() doesn't seem too good... First, I'm not sure what it's meant by 'fully' capitalizing a String. I thought you meant converting a String to uppercase. Turns out you are just capitalizing the first letter. So why not call that then?

private static String capitalizeFirst(String input) {
    // ...
}

Unless you are constructing Strings from byte/char arrays, with an optional Charset, you should (almost) never need to instantiate a String by doing new String(). Then, using the toLowerCase() and toUpperCase() methods...

private static String capitalizeFirst(String input) {
    // handle null or "" Strings if required
    return input.substring(0, 1).toUpperCase() + input.substring(1).toLowerCase();
}

If you want to cater for Locale-specific conversions, there are methods for that too.

Last, you need a much better name for your print (?) p() method... what is it supposed to do actually, anyways? The returned object doesn't appear to be used in any way.