Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Quick suggestions...

  • xIsDivisibleByY(): might be better to leave it as isDivisble(), the point being it's relatively straightforward to infer the ordering. x and y isn't much better, since they can become misleading if you decide to rename your method arguments a, b or numerator, denominator etc.

  • The body of xIsDivisibleByY() can also be return y != 0 && x % y == 0.

  • You can simply put xIsDivisibleByY(n + n + 1, 7) for isWow().

  • Are you really expected to show your implementation for prime-checking? :D Since your upper bound is only 113, I'm kind of tempted to just use a pre-computed list of primes and call it a day...

  • In any case, a slight optimization for your prime-checking can be to first test if it's divisible by 2 or not. If not, you just need to iterate through the odd numbers up to (削除) sqrtOfN (削除ここまで) threshold: a decidedly better name since sqrtOfN is never really the actual square root, is it? ;)

  • As mentioned in @janos's @janos's answer, use a StringBuilder instead of rolling your own addToList() method, which is also named wrongly anyways since it isn't adding to any list.

  • boolean printDirectly: It's commendable that you spot the slightly odd requirement of having to print to System.out and returning an ArrayList. However, you should go a step to further to really illustrate how your internal method is observing this 'separation-of-concerns'-like behavior as such:

     // would be great to return as a List, 
     // unfortunately caller expects an ArrayList to return itself too
     private static ArrayList<String> getResult(startInclusive, endInclusive) {
     ArrayList<String> result = new ArrayList<>();
     // ...
     return result;
     }
     public static ArrayList<String> iterate() {
     ArrayList<String> result = getResult(0, 113);
     for (String current : result) {
     System.out.println(current);
     }
     return result;
     }
    

Quick suggestions...

  • xIsDivisibleByY(): might be better to leave it as isDivisble(), the point being it's relatively straightforward to infer the ordering. x and y isn't much better, since they can become misleading if you decide to rename your method arguments a, b or numerator, denominator etc.

  • The body of xIsDivisibleByY() can also be return y != 0 && x % y == 0.

  • You can simply put xIsDivisibleByY(n + n + 1, 7) for isWow().

  • Are you really expected to show your implementation for prime-checking? :D Since your upper bound is only 113, I'm kind of tempted to just use a pre-computed list of primes and call it a day...

  • In any case, a slight optimization for your prime-checking can be to first test if it's divisible by 2 or not. If not, you just need to iterate through the odd numbers up to (削除) sqrtOfN (削除ここまで) threshold: a decidedly better name since sqrtOfN is never really the actual square root, is it? ;)

  • As mentioned in @janos's answer, use a StringBuilder instead of rolling your own addToList() method, which is also named wrongly anyways since it isn't adding to any list.

  • boolean printDirectly: It's commendable that you spot the slightly odd requirement of having to print to System.out and returning an ArrayList. However, you should go a step to further to really illustrate how your internal method is observing this 'separation-of-concerns'-like behavior as such:

     // would be great to return as a List, 
     // unfortunately caller expects an ArrayList to return itself too
     private static ArrayList<String> getResult(startInclusive, endInclusive) {
     ArrayList<String> result = new ArrayList<>();
     // ...
     return result;
     }
     public static ArrayList<String> iterate() {
     ArrayList<String> result = getResult(0, 113);
     for (String current : result) {
     System.out.println(current);
     }
     return result;
     }
    

Quick suggestions...

  • xIsDivisibleByY(): might be better to leave it as isDivisble(), the point being it's relatively straightforward to infer the ordering. x and y isn't much better, since they can become misleading if you decide to rename your method arguments a, b or numerator, denominator etc.

  • The body of xIsDivisibleByY() can also be return y != 0 && x % y == 0.

  • You can simply put xIsDivisibleByY(n + n + 1, 7) for isWow().

  • Are you really expected to show your implementation for prime-checking? :D Since your upper bound is only 113, I'm kind of tempted to just use a pre-computed list of primes and call it a day...

  • In any case, a slight optimization for your prime-checking can be to first test if it's divisible by 2 or not. If not, you just need to iterate through the odd numbers up to (削除) sqrtOfN (削除ここまで) threshold: a decidedly better name since sqrtOfN is never really the actual square root, is it? ;)

  • As mentioned in @janos's answer, use a StringBuilder instead of rolling your own addToList() method, which is also named wrongly anyways since it isn't adding to any list.

  • boolean printDirectly: It's commendable that you spot the slightly odd requirement of having to print to System.out and returning an ArrayList. However, you should go a step to further to really illustrate how your internal method is observing this 'separation-of-concerns'-like behavior as such:

     // would be great to return as a List, 
     // unfortunately caller expects an ArrayList to return itself too
     private static ArrayList<String> getResult(startInclusive, endInclusive) {
     ArrayList<String> result = new ArrayList<>();
     // ...
     return result;
     }
     public static ArrayList<String> iterate() {
     ArrayList<String> result = getResult(0, 113);
     for (String current : result) {
     System.out.println(current);
     }
     return result;
     }
    
Source Link
h.j.k.
  • 19.3k
  • 3
  • 37
  • 93

Quick suggestions...

  • xIsDivisibleByY(): might be better to leave it as isDivisble(), the point being it's relatively straightforward to infer the ordering. x and y isn't much better, since they can become misleading if you decide to rename your method arguments a, b or numerator, denominator etc.

  • The body of xIsDivisibleByY() can also be return y != 0 && x % y == 0.

  • You can simply put xIsDivisibleByY(n + n + 1, 7) for isWow().

  • Are you really expected to show your implementation for prime-checking? :D Since your upper bound is only 113, I'm kind of tempted to just use a pre-computed list of primes and call it a day...

  • In any case, a slight optimization for your prime-checking can be to first test if it's divisible by 2 or not. If not, you just need to iterate through the odd numbers up to (削除) sqrtOfN (削除ここまで) threshold: a decidedly better name since sqrtOfN is never really the actual square root, is it? ;)

  • As mentioned in @janos's answer, use a StringBuilder instead of rolling your own addToList() method, which is also named wrongly anyways since it isn't adding to any list.

  • boolean printDirectly: It's commendable that you spot the slightly odd requirement of having to print to System.out and returning an ArrayList. However, you should go a step to further to really illustrate how your internal method is observing this 'separation-of-concerns'-like behavior as such:

     // would be great to return as a List, 
     // unfortunately caller expects an ArrayList to return itself too
     private static ArrayList<String> getResult(startInclusive, endInclusive) {
     ArrayList<String> result = new ArrayList<>();
     // ...
     return result;
     }
     public static ArrayList<String> iterate() {
     ArrayList<String> result = getResult(0, 113);
     for (String current : result) {
     System.out.println(current);
     }
     return result;
     }
    
lang-java

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