FizzBuzz-like Assignment

Question

I have an assignment for my Java programming class that is kind of an expanded FizzBuzz-like question:

9. Write a program that iterates through numbers from 0 to 113 using a loop. Print the numbers, one number per line. As you print each number, say x, also print the following when appropriate, separated by commas:

   • If the number is odd, print "x is odd"
   • If the number is divisible by 5, print "hi five"
   • If the total of a number (x) and its subsequent number (x+1) is a value divisible by 7, print "wow"
   • If the number is prime, print "prime".

My main concerns are:

• Am I interpreting the condition for part 3 (isWow) correctly?
• Is there a better way of organizing the function that doesn't require a cascade of if statements?
• For an acedemic context, are the comments appropriate?
• Anything else that could be improved upon, including style.

---

package comp268.q9;
import java.util.ArrayList;
public class Number {
 //I thought I might as well generalize the pattern.
 public static boolean xIsDivisibleByY(int x, int y) {
 if (y == 0) return false;
 return x % y == 0;
 }
 public static boolean isDivisibleBy5(int n) {
 return xIsDivisibleByY(n, 5);
 }
 public static boolean isDivisibleBy7(int n) {
 return xIsDivisibleByY(n, 7);
 }
 public static boolean isOdd(int n) {
 return !xIsDivisibleByY(n, 2);
 }
 public static boolean isWow(int n) {
 return isDivisibleBy7(n + n + 1); 
 }
 //A very simple prime checker.
 //The only optimization I'm using is that I'm only checking factors up to
 // the sqrt of x, since past there we're finding the "mirror" of factors
 // we've already found.
 public static boolean isPrime(int n) {
 //Numbers <= 1 are not considered prime
 if ( n < 2 ) return false;
 //Caching the sqrt since it's relatively expensive to compute.
 int sqrtOfN = (int)Math.sqrt(n) + 1;
 for (int factorToCheck = 2; factorToCheck < sqrtOfN; factorToCheck++) {
 if (xIsDivisibleByY(n, factorToCheck)) {
 return false;
 }
 }
 return true;
 }
 //I could just mutate the passed String.
 //Avoiding side-effects seemed cleaner though.
 private static String addToList(String str, String addition) {
 return str + ", " + addition;
 }
 //The instructions suggest that the method should print the messages directly.
 //To me, that seems like unnecessary side-effects since we're returning a list of messages.
 //I added a flag parameter to control whether or not the method prints directly.
 public static ArrayList<String> iterate(int start, int end, boolean printDirectly) {
 ArrayList<String> allMessages = new ArrayList<String>();
 for (int n = start; n <= end; n++) {
 String curMessages = Integer.toString(n);
 //The instructions suggest that the number itself should be included
 // in the message, even if it adds redundancy; unless it's intent was 
 // for the message to literally be "x is odd".
 if (isOdd(n)) {
 curMessages = addToList(curMessages, n + " is odd");
 }
 if (isDivisibleBy5(n)) {
 curMessages = addToList(curMessages,"hi five");
 }
 if (isWow(n)) {
 curMessages = addToList(curMessages, "wow");
 }
 if (isPrime(n)) {
 curMessages = addToList(curMessages, "prime");
 }
 if (printDirectly) {
 System.out.println(curMessages);
 }
 allMessages.add(curMessages);
 }
 return allMessages;
 }
 //Default version to abide by the API
 public static ArrayList<String> iterate() {
 return iterate(0, 113, true);
 }
}

Note that I added the "default version" since they wanted iterate to accept 0 arguments. The following functions must be supplied exactly:

enter image description here

So, for these methods, comments regarding naming are unnecessary.

Also note that by the assignment question (unless I'm misinterpreting it), iterate must print as it goes; it can't just return the results, which would be ideal.

• Captain Man: Your comment on addToList says you could mutate the String, but in Java Strings are immutable.
• Carcigenicate: "Mutate" was a poor word. Unless I was super tired this morning, I thought I was able to reassign the String to avoid needing to return it.
• Carcigenicate: @CaptainMan Whoops, I guess I was tired. I don't know what I saw that made me think I could do that :/ Thanks.
• Captain Man: Even if you did str = ... instead of return ... you wouldn't mess with the param given. Java is "pass by value". If you could do something like str.setContent(...) then you would be doing what you thought you were avoiding, but Strings are immutable and can't be modified without reassigning them, and reassigning one variable won't mess with the one that called it. :)
• Carcigenicate: @h.j.k. Lol. If you recall from my previous question here, I'm being super critical of almost every assignment they've given me. I don't think I've done one yet that hasn't had non-sensical requirements.

Answer 1

Am I interpreting the condition for part 3 (isWow) correctly?

I think so.

Is there a better way of organizing the function that doesn't require a cascade of if statements?

There is a way, but I'm not sure it's worth it. For example, you could:

1. Create an interface with a single method that takes an integer and returns a string
2. Create classes that implement this interface, each class applying one of the prescribed check, and returning a non-empty string when applicable
3. Put the classes in a list
4. As you iterate over the numbers, replace the cascade of if statements with a loop over the list of the implementing classes

But I don't think it's worth it. The problem is not interesting enough to overengineer it.

For an acedemic context, are the comments appropriate?

Honestly, your code is so easy to read that I completely disregarded the comments, they were invisible to me until now. Now that I read them, I see that some are too chatty, most are unnecessary, and some are completely incorrect. ("I could just mutate the passed String." -- no you cannot mutate Strings in Java.) Overall, the comments hurt you more than help you. You don't need them, delete them.

Anything else that could be improved upon, including style.

• Refer to types using interfaces instead of implementations: use List instead of ArrayList in type declarations

• When doing a lot of string concatenations, consider using a StringBuilder

• Most of your names are descriptive, but some are really not good:

  • addToList sounds like adding to a list, but concats the params and returns a String
  • iterate : the name suggests a void method iterating over a collection, but that's not what's happening here. The term "iterate" is typically used in the iterator pattern, and it's somewhat inappropriate here, especially because the method returns a list. Perhaps checkNumbersInRange would have been better.
  • xIsDivisibleByY feels really awkward. I think divisibleBy(x, y) would be just fine

• iterate does too many things: it returns a list, and sometimes it also prints. The printing doesn't belong there.

• It would be good to extract the actions in the main loop body to a separate method (with the cascade of if statements)

• Carcigenicate: Thank you. Unfortunately, most of the names were picked by the instructor. They're requiring me to adhere to an API (including exact names, and parameters; including the explicit use of ArrayList). I'll keep that in mind for personal projects though. And they're requiring that iterate print the results it goes; even though it's returning the results. And in a case like you mentioned for concatenation, aren't the +s automatically translated into StringBuilder operations? I'll add the API picture that was included in the assignment showing the required names. Thanks again.
• janos: You don't have to be defensive. This is my code review, it doesn't matter if parts of it were forced upon you by requirements.
• Carcigenicate: Sorry, that's not the tone I was intending to take. I wrote that while ordering lunch. I didn't mean to sound unappreciative.
• Captain Man: @Carc if you are supposed to be adherring to an API, were you meant to extend an abstract class or implement an interface and perhaps forgot?
• Carcigenicate: @CaptainMan No. They're referring to what's in the picture as the API. I think they're marking each assignment by running the program using a premade main, so they're expecting certain signatures.

Answer 2

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;
  }
  

• Carcigenicate: Thanks again. One question I have though (and I'll tag @janos here aswell), is that I was under the impression from a few posts that the compiler is pretty good at automatically converting "plain" String concatenation into StringBuilder operations. Is this not the case? And good point in the last recommendation. I had it stuck in my head that it should print the results live. I didn't even consider just using an aux function to wrap the main work-horse function. Thank you.
• h.j.k.: @Carcigenicate yeah, but I don't know if the compiler will be observant enough to use one StringBuilder instance per iteration for all your if statements. Worst-case scenario is that it's going to create four instances. Not a big deal for your assignment though...
• Carcigenicate: Ok, thank you. I'll look further into how the compiler transforms concatenation into a StringBuilder.
• h.j.k.: @Carcigenicate Don't sweat the small stuff. ;)