Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

You probably can do away with the throws declaration for the unchecked IllegalArgumentException, and some some answers answers over at StackOverflow back this up.

You probably can do away with the throws declaration for the unchecked IllegalArgumentException, and some answers over at StackOverflow back this up.

You probably can do away with the throws declaration for the unchecked IllegalArgumentException, and some answers over at StackOverflow back this up.

deleted 15 characters in body
Source Link
h.j.k.
  • 19.4k
  • 3
  • 37
  • 93
public static class Suit {
 private String name;
 private static final List<String> stringNames = Arrays.asList(
 "Hearts",
 "Diamonds",
 "Clubs",
 "Spades"
 );
 public Suit(String name) {
 String capitalizedresult = capitalizeFirstLetter(name);
 if (!stringNames.contains(capitalizedresult)) {
 throw new IllegalArgumentException("Invalid suit name.");
 }
 this.name = capitalized;result;
 }
}

Finally, by using a List and its List.contains(Object) method, I can easily determine whether the input is accepted or not. The else part is also redundant as we will already bybe throwing the Exception for invalid names.

public static class Suit {
 private String name;
 private static final List<String> stringNames = Arrays.asList(
 "Hearts",
 "Diamonds",
 "Clubs",
 "Spades"
 );
 public Suit(String name) {
 String capitalized = capitalizeFirstLetter(name);
 if (!stringNames.contains(capitalized)) {
 throw new IllegalArgumentException("Invalid suit name.");
 }
 this.name = capitalized;
 }
}

Finally, by using a List and its List.contains(Object) method, I can easily determine whether the input is accepted or not. The else part is also redundant as we will already by throwing the Exception for invalid names.

public static class Suit {
 private String name;
 private static final List<String> stringNames = Arrays.asList(
 "Hearts",
 "Diamonds",
 "Clubs",
 "Spades"
 );
 public Suit(String name) {
 String result = capitalizeFirstLetter(name);
 if (!stringNames.contains(result)) {
 throw new IllegalArgumentException("Invalid suit name.");
 }
 this.name = result;
 }
}

Finally, by using a List and its List.contains(Object) method, I can easily determine whether the input is accepted or not. The else part is also redundant as we will already be throwing the Exception for invalid names.

Source Link
h.j.k.
  • 19.4k
  • 3
  • 37
  • 93

It's me again. :)

public static <T> void p(T output) {
 System.out.println(output);
}
public static <T> void pnln(T output) {
 System.out.print(output);
}

I still frown at this, because you're making up the tediousness of typing appropriately-named method names with too-simplified ones, when this can easily be mitigated using snippets in Sublime, your editor of choice.

Furthermore, if I may say so, it's now even worse as what looks like printing a line pnln() is not (System.out.print()), and what looks like a non-newline-terminated operation p() is (System.out.println()). This is... layered confusion, I'm afraid. You also definitely do not need the generic typing here, so leaving your method argument as Object will work equally well.

enums are implicitly static final, so you can drop those modifiers for your enum declarations.

If for some reason you still prefer your array-based approach to create new instances of Rank and Suit classes, may I suggest using a List instead so that comparisons with valid inputs can be done in a more compact and arguably efficient way? Let's use Suit as an example:

public static class Suit {
 private String name;
 private static final String[] string_names = {
 "Hearts",
 "Diamonds",
 "Clubs",
 "Spades"
 };
 public Suit(String name) throws IllegalArgumentException {
 int i = 0;
 String n = capitalizeFirstLetter(name);
 while (i < string_names.length && !n.equals(string_names[i])) {
 i++;
 }
 if (i == string_names.length) {
 throw new IllegalArgumentException("Invalid suit name.");
 } else {
 this.name = n;
 }
 }
}

Here, you have to explicitly loop through your array in order to validate the String input. When you use Arrays.asList() however...

public static class Suit {
 private String name;
 private static final List<String> stringNames = Arrays.asList(
 "Hearts",
 "Diamonds",
 "Clubs",
 "Spades"
 );
 public Suit(String name) {
 String capitalized = capitalizeFirstLetter(name);
 if (!stringNames.contains(capitalized)) {
 throw new IllegalArgumentException("Invalid suit name.");
 }
 this.name = capitalized;
 }
}

I forgot to point this out earlier, but Java's standard naming convention is camelCase, not snake_case.

You probably can do away with the throws declaration for the unchecked IllegalArgumentException, and some answers over at StackOverflow back this up.

Finally, by using a List and its List.contains(Object) method, I can easily determine whether the input is accepted or not. The else part is also redundant as we will already by throwing the Exception for invalid names.

lang-java

AltStyle によって変換されたページ (->オリジナル) /