5
\$\begingroup\$

I have been asked to implement this and completed that. There was also a request to apply the SOLID principles and use a CLEAN CODE approach.

Write a program that prints the numbers from 1 to 100. But for multiples of three print "Fizz" instead of the number and for the multiples of five print "Buzz". For numbers which are multiples of both three and five print "FizzBuzz".

Here is the code that I provided

using System;
                    
public class Program
{
    public static void Main()
    {
        for(int i=1; i<= 100; i++) 
        {
            Console.WriteLine(getStringForNumber(i));
        }
        
    }
    
    public static string getStringForNumber(int i) {
        bool isMult3 = (i%3 == 0) ;
        bool isMult5 = (i%5 == 0) ;
        
        string line = "" + i;
        //For numbers which are multiples of both three and five print "FizzBuzz".
        if(isMult3 && isMult5) {
            line =  "FizzBuzz";
        }
        else {
            //But for multiples of three print "Fizz" instead of the number 
            if(isMult3) {
                line =  "Fizz";
            }
            //and for the multiples of five print "Buzz". 
            else if(isMult5) {
                line =  "Buzz";
            }   
        }
        return line ;
    }
}
\$\endgroup\$
4
  • 4
    \$\begingroup\$ Now that the code works as intended I have reopened the post. I also changed the title so that it describes what the code does per site convention: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. \$\endgroup\$ Commented Mar 2 at 16:12
  • 2
    \$\begingroup\$ Study the many (many) FizzBuzz threads here on CodeReview. I'm positive you will find things to delight and astound your pedagogic pursuit of the elusive SOLID principles and ... CLEAN CODE \$\endgroup\$
    – radarbob
    Commented Mar 3 at 22:55
  • 2
    \$\begingroup\$ None of the SOLID principles apply to FizzBuzz. Either the interviewer was very ignorant of SOLID principles, or they were testing you to see if you would push back, or that is an employer to be avoided. SOLID principles are for large scale object oriented design; it's a OO-programming-in-the-large guideline. It makes no sense to apply these principles to what could be a one-line program. \$\endgroup\$ Commented Mar 5 at 23:49
  • 2
    \$\begingroup\$ It makes no sense to apply these principles to what could be a one-line program -> Well, there is always the jewel of Java Oriented Programming, Java Enterprise FizzBuzz!! \$\endgroup\$
    – radarbob
    Commented Mar 10 at 23:00

4 Answers 4

4
\$\begingroup\$

FizzBuzz has two main characteristics:

  1. The problem is extremely simple. And so acts as a "does the person understand any programming?" filter.
  2. The problem isn't really something you apply principles too. And shows if people overcomplicate problems.

  1. Lets move the adding the number to output to the bottom of the function:

    string line = "" + i;
    
    string line = "";
    ...
    return line != "" ? line : "" + i;
    
  2. Lets condense the ifs by using line += "...".

    if(i % 3 == 0) {
        line += "Fizz";
    }
    if(i % 5 == 0) {
        line += "Buzz";
    }
    
public static string getStringForNumber(int i) {
    string line = "";
    if (i % 3 == 0) {
        line += "Fizz";
    }
    if (i % 5 == 0) {
        line += "Buzz";
    }
    return line != "" ? line : "" + i;
}
  1. We can convert the ifs to ternaries.

    if (i % 3 == 0) {
        line += "Fizz";
    }
    
    line += i % 3 == 0 ? "Fizz" : "";
    
  2. We can merge the two ternaries into the assignment of line.

    string line = "";
    line += i % 3 == 0 ? "Fizz" : "";
    line += i % 5 == 0 ? "Buzz" : "";
    
    string line =
        (i % 3 == 0 ? "Fizz" : "")
      + (i % 5 == 0 ? "Buzz" : "");
    
  3. In some languages (not C#) you can remove the line variable by using the languages or operator.

    def fizz_buzz__line(i: int) -> str:
        return (
            (
                ("Fizz" if i % 3 == 0 else "")
              + ("Buzz" if i % 5 == 0 else "")
            )
            or f"{i}"
        )
    
public static string getStringForNumber(int i) {
    string line =
        (i % 3 == 0 ? "Fizz" : "")
      + (i % 5 == 0 ? "Buzz" : "");
    return line != "" ? line : "" + i;
}

Since FizzBuzz is typically an interview question to filter out people, I would use issues as markers against the person.

  1. The code is not consistent.

    bool isMult3 = (i%3 == 0) ;
    
    string line = "" + i;
    
    line =  "FizzBuzz";
    
    return line ;
    
  2. The comments are examples of ones beginner programmers write.

  3. DRYing bool isMult3 = (i%3 == 0) ; is a bit excessive. And not DRYing the code duplication of isMult3's usage is note worthy.

  4. Having whitespace after lines of code is alarming.

    else if(isMult5) {
        line =  "Buzz";
    }   // <- whitespace here
    
  5. The function name getStringForNumber doesn't in any way describe the function is to do with FizzBuzz.

\$\endgroup\$
4
  • \$\begingroup\$ Here are my comments (1/2) - The comments were taken from the comments provided in the requirements and pasted there (having comments is not for beginners only) - DRY is not about "not using the same variable in multiple places) the places where isMult3 is being used is because they are different conditions, in fact that is the reason I have decided to store it and give it a name to make it more readable - "Having whitespace after lines of code is alarming" where is that mention in clean code practices ? separating codes of block is to make code more readable and the compiler ignores them \$\endgroup\$ Commented Mar 4 at 15:27
  • \$\begingroup\$ 2/2 - "getStringForNumber" has everything to do with FizzBuzz because given a numbers returns the line of text (string) to show - Finally when you say that "The code is not consistent." is no very clear what you mean by that also you made a bunch of observations but your complete code was not provided - here is my own answer codereview.stackexchange.com/a/290865/29384 \$\endgroup\$ Commented Mar 4 at 15:45
  • \$\begingroup\$ @MauricioGraciaGutierrez "comments is not for beginners only" you have misunderstood the sentence. You can see I DRYed isMult3's usage. "where is that mention in clean code practices" I can give you examples related to Python - PEP8 and accompanying linters. "separating codes of block ..." I don't understand what you are trying to communicate. ""getStringForNumber" has everything to do with FizzBuzz" you are entitled to your opinion, you can see both answers disagree with you. "your complete code was not provided" You can see the complete code after 2 and 5. \$\endgroup\$
    – Peilonrayz
    Commented Mar 4 at 16:22
  • \$\begingroup\$ @Peilonrayz all answers are different, including comments that there is not enough code or makes no sense to apply SOLID or CLEAN CODE principles, that means that each of us can choose the ones that we like, diversity of answer does make me wrong or you right ;-) \$\endgroup\$ Commented Mar 22 at 0:14
2
\$\begingroup\$

I respectfully disagree with Peilonrays about one thing: if I got a question like this, I would interpret it to mean that I’m expected to demonstrate some familiarity with both SOLID and Clean Code, as well as how to use them sensibly in moderation.

First, as for the code itself, we can make it a lot nicer by remembering that a number is divisible by both 3 and 5 if and only if it’s divisible by 15. There are a few clever FizzBuzz programs out there, but I generally like to write static single assignments with ternary expressions, so I’ll go with that:

using System;

public class Program
{
    public static void Main()
    {
        for(int i=1; i<= 100; i++) 
        {
            Console.WriteLine(ToFizzBuzz(i));
        }
    }
    
    public static string ToFizzBuzz(int i) {
        return (i%15 == 0) ? "FizzBuzz" :
               (i%3 == 0)  ? "Fizz"     :
               (i%5 == 0)  ? "Buzz"     :
                             i.ToString();
    }
}

The helper function is now a single statement. We’ve eliminated all temporary variables and concatenation. This is also pretty efficient. Because of the short-circuiting of the nested ternaries. We only check divisibility by 3 or 5, or convert i to a string, if we need to. And in optimal order: since there are twice as many numbers divisible by 3 but not 15 as by 5 but not 15, it is more efficient to check i%3 before i%5.

That said, no reasonable interviewer is going to expect you to actually come up with all the clever number-theory tricks for these problems on the spot.

One way in which Clean Code is relevant here is that it says to know the coding conventions of the language. For C#, these say, “Use PascalCase for class names and method names.” I therefore renamed your helper method in accordance with Clean Code.

At this point, I’d probably try to tell the interviewer, “The S in SOLID is for Single Responsibility Principle, and everything has a Single Responsibility. The O is for Open for Extension/Closed for Modification, and a maintainer could easily extend it by adding some other transformation or using the string in another way. But ToFizzBuzz does one thing that doesn’t need to be modified. The problem is too simple for the other three, like substitution, interfaces and dependency inversion, to really apply.” If I’d looked it up recently and had them fresh in my memory, that is. I honestly don’t do a lot of OOP these days.

\$\endgroup\$
7
  • \$\begingroup\$ "I’m expected to demonstrate some familiarity with both SOLID and Clean Code". I think we can both agree both our solutions have improved Clean Code over the OP's. Do you believe you have improved the SOLID of the function? I have read the final paragraph where you somewhat tie SOLID to the code, but unlike the Clean Code example I can't see where you've explained an improvement in SOLID. I'm happy and would like to be wrong, but I can't understand how I am. If your only contestation is "familiarity" then sure, let's agree I'm wrong. \$\endgroup\$
    – Peilonrayz
    Commented Mar 4 at 2:06
  • \$\begingroup\$ @Peilonrayz I was thinking of, “The problem isn't really something you apply principles too. And shows if people overcomplicate problems.” If you in fact agree that the interviewer does want to see the candidate find a way to apply the principles, great! \$\endgroup\$
    – Davislor
    Commented Mar 4 at 2:36
  • \$\begingroup\$ @Peilonrayz But also, if your honest opinion is that the principle isn’t applicable to this problem, you can demonstrate your understanding of the principle by explaining that. \$\endgroup\$
    – Davislor
    Commented Mar 4 at 2:40
  • \$\begingroup\$ I actually said to the interviewer that he could ask me anything regarding SOLID and CLEAN CODE but he refused - thanks for you very clear and empatic response here is my approach - codereview.stackexchange.com/a/290865/29384 \$\endgroup\$ Commented Mar 4 at 15:44
  • \$\begingroup\$ @Peilonrayz Since I wasn’t clear, then: I didn’t add or remove any helper functions, or change what their parameters or return values. So I think the code itself was no more or less SOLID. I included an example of what I might tell an interviewer to demonstrate how the code relates to SOLID. I do think of this refactoring as “cleaner” even simpler and easier to understand, than the original. Which wasn’t bad! \$\endgroup\$
    – Davislor
    Commented Mar 4 at 21:10
1
\$\begingroup\$

Here is my own approach to follo DRY, SOLID and CLEAN CODE

using System;

namespace FizzBuzz
{
    public class Program
    {
        public static void Main()
        {
            for (int i = 1; i <= 100; i++)
            {
                Console.WriteLine(FizzBuzz(i));
            }
        }

        public static bool IsMultipleOf(int number, int divisor)
        {
            return (number % divisor == 0);
        }

        public static string FizzBuzz(int number)
        {
            if (IsMultipleOf(number, 15))
            {
                return "FizzBuzz";
            }

            if (IsMultipleOf(number, 3))
            {
                return "Fizz";
            }

            if (IsMultipleOf(number, 5))
            {
                return "Buzz";
            }

            return number.ToString();
        }
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ You’ve seen the coding style I personally prefer. If we actually needed an extremely speedy FizzBuzz, the way to do that would be to print out the sequence in groups of 15, eliminating all the conditional branches and divisibility checks. Or if we needed fast FizzBuzz transformation in arbitrary order, I’d write a lookup table of nullable string constants, with 15 elements, and look up i%15. \$\endgroup\$
    – Davislor
    Commented Mar 4 at 16:03
  • \$\begingroup\$ The question is not really about performance, and SOLID and CLEAN CODE in theory is not about personal taste. \$\endgroup\$ Commented Mar 4 at 16:06
  • 2
    \$\begingroup\$ Right, it’s a serviceable answer. In my personal opinion, the built-in % operator is shorter and sweeter than a helper function IsMultipleOf. \$\endgroup\$
    – Davislor
    Commented Mar 4 at 16:09
  • \$\begingroup\$ And that was in my original question, but for the recruiter that was not clean code ;-) \$\endgroup\$ Commented Mar 5 at 14:37
1
\$\begingroup\$

Your application is too small to be able to meaningfully talk about SOLID and/or CLEAN CODE.

The entire application can be rewritten into a single statement:

Enumerable.Range(1, 100)
    .Select(x => x switch
            {
                _ when x % 15 == 0 => "FizzBuzz",
                _ when x % 3 == 0 => "Fizz",
                _ when x % 5 == 0 => "Buzz",
                _ => x.ToString()
            })
    .ToList()
    .ForEach(Console.WriteLine);

Due to the current language restrictions (you can use only <, >, <=, or >= as relational operator) you can't write the switch expression like this:

Enumerable.Range(1, 100)
    .Select(x => x switch
            {
                % 15 == 0 => "FizzBuzz",
                % 3 == 0 => "Fizz",
                % 5 == 0 => "Buzz",
                _ => x.ToString()
            })
    .ToList()
    .ForEach(Console.WriteLine);
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Here's my favorite "one-liner": Console.WriteLine(string.Join( Environment.NewLine, Enumerable.Range(1, 100) .Select(n => (n, o: string.Empty)) .Select(x => x.n % 3 == 0 ? (x.n, o: "Fizz") : x) .Select(x => x.n % 5 == 0 ? (x.n, o: x.o + "Buzz") : x) .Select(x => string.IsNullOrEmpty(x.o) ? (x.n, o: x.n.ToString()) : x) .Select(x => x.o))); \$\endgroup\$ Commented Mar 11 at 20:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.