2
\$\begingroup\$

I have the following task at hand and I have completed it and it is working fine. Since I am new to design patterns, I would like to take your opinions on how I can improve the code quality in terms of incorporating more reusable code and implementing much efficient design, etc.

The task is to give an input number and generate a bunch of numeric sequences like:

  1. All the numbers below that number
  2. All the even numbers below that number
  3. All the odd numbers below that number
  4. All the Fibonacci numbers below that number
  5. All the magic numbers below that number and so on

    public  class AllNumberOutput
        {
            public int InputNumber { get; set; }
            public List<string> NumList { get; set; }
            public List<string> OutputList (int inputNumber,List<string> numList=null)
            {
                NumList = numList;
                InputNumber = inputNumber;
                for (int i = 1; i <= inputNumber; i++)
                {
                    numList.Add(i.ToString());
                }
                return numList;
            }
      public class AllEvenNumbers:AllNumberOutput
    {       
        public List<string> OutputList(int inputNumber, List<string> numList)
        {
           this.NumList = numList;
           this.InputNumber = inputNumber;
           List<int> tempList= numList.Select(s => int.Parse(s)).ToList();
           tempList.RemoveAll(n => n % 2 != 0);
           numList= tempList.Select(s => s.ToString()).ToList();
           return numList;
        }
    }     

        } 
 public class AllOddNumbers:AllNumberOutput
    {
        public List<string> OutputList(int inputNumber, List<string> numList)
        {
            this.NumList = numList;
            this.InputNumber = inputNumber;
            List<int> tempList = numList.Select(s => int.Parse(s)).ToList();
            tempList.RemoveAll(n => n % 2 == 0);
            numList = tempList.Select(s => s.ToString()).ToList();
            return numList;
        }
    }
 public List<string> OutputList( int inputNumber, List<string> numList)
        {
            this.NumList = numList;
            //this.InputNumber = inputNumber;
            List<int> tempList = numList.Select(s => int.Parse(s)).ToList();
            for (int i = 0; i < numList.Count; i++)
            {
                if (tempList[i] % 3 == 0 && tempList[i] % 5 != 0)
                {
                    NumList[i] = "C";
                }
                if (tempList[i] % 3 != 0 && tempList[i] % 5 == 0)
                {
                    NumList[i] = "E";
                }
                if (tempList[i] % 3 == 0 && tempList[i] % 5 == 0)
                {
                    NumList[i] = "Z";
                }

            }
            return NumList;
        }
 public class AllFibonacciNumbers : AllNumberOutput
    {
        public  List<string> OutputList(int inputNumber, List<string> numList)
        {
            this.NumList = numList;
            this.InputNumber = inputNumber;
            List<int> tempList = new List<int>() ;// = numList.Select(s => int.Parse(s)).ToList();
            for (int i = 0; i <= inputNumber; i++)
            {
                if (IsFibonacci(i))
                    tempList.Add(i);
            }
           // tempList.RemoveAll(n => !IsFibonacci(n));
            numList = tempList.Select(s => s.ToString()).ToList();
            return numList;
        }
        public static bool IsFibonacci(int num)
        {
            int a = 0;
            int b = 1;
            if (num == a || num == b) return true;
            int c = a + b;
            while (c <= num)
            {
                if (c == num) return true;
                a = b;
                b = c;
                c = a + b;
            }

            return false;
        }
    }

Now, from the client side, I am instantiating and calling them like below:

AllNumberOutput allNumOutput = new AllNumberOutput();
        AllOddNumbers oddNumOutPut = new AllOddNumbers();
        AllEvenNumbers evenOutput = new AllEvenNumbers();
        AllDivisibleByNumbers divOutput = new AllDivisibleByNumbers();
        AllFibonacciNumbers fiboutput = new AllFibonacciNumbers();
        public List<string> NumList=new List<string>();
          //All numbers 
            List<string> resultList = allNumOutput.OutputList(100, NumList);
          // All oddnumbers 
            List<string> oddList=oddNumOutPut.OutputList(100, resultList);
            // All even numbers 
            List<string> evenList=evenOutput.OutputList(100, resultList);
            /* All numbers , except  when,  
            A number is a multiple of 3 output C, and when,  
            A number is a multiple of 5 output E, and when,   
            A number is a multiple of both 3 and 5 output Z,  
            */
            List<string> conditionalList= divOutput.OutputList(100,resultList);
            // All fibonacci numbers    
            List<string> fiblList= fiboutput.OutputList(100, resultList);
\$\endgroup\$
3
\$\begingroup\$

What you're doing is generating multiple sequences up to a known value. That implies a sequence class. Here's an example:

public static class Sequences {
    public static IEnumerable<int> Integers() {
        return Enumerable.Range(0, int.MaxValue);
    }

    public static IEnumerable<int> Even() {
        return Integers().Where(i => i % 2 == 0);
    }

    public static IEnumerable<int> Odd() {
        return Integers().Where(i => i % 2 != 0);
    }

    public static IEnumerable<long> Fibonacci(long a = 1, long b = 1) {
        do {
            yield return a;
            long c = a + b;
            a = b;
            b = c;
        } while (true);
    }
}

Those are lazily generated and all you have to do is filter them down to the size you want or transform them like your magic list example. Let's look at that first:

public static class DomainExtensions {
    public static IEnumerable<string> ToMagic(this IEnumerable<int> list) {
        return list.Select(transform);
    }

    private static string transform(int i) {
        if (i % 3 == 0 && i % 5 == 0)
            return "Z";

        if (i % 3 == 0)
            return "C";

        if (i % 5 == 0)
            return "E";

        return i.ToString();
    }
}

To output...

var number = 100;
var evenUnder100 = Sequences.Even().TakeWhile(i => i < number);
var fibonacciUnder100 = Sequences.Fibonacci().TakeWhile(i => i < number);
var magicUnder100 = Sequences.Integers().TakeWhile(i => i < number).ToMagic();

Console.WriteLine(string.Join(",", evenUnder100));
Console.WriteLine(string.Join(",", fibonacciUnder100));
Console.WriteLine(string.Join(",", magicUnder100));

Some of the key takeaways:

  • A class is a thing. Try not to make classes that are actually instances of some thing. The hint here is your class names sound like variable names, and indeed they end up being that.
  • Handling sequences/collections is super important and useful - most languages have facilities for doing lots of work with it. In C#, that is LINQ. It's powerful. Get really familiar with it and you won't regret it.
  • Contrary to popular belief, static functions can be your friend if it does not depend on or mutate external state. You don't need to create instances of 'stuff' just to get code to run.
\$\endgroup\$
  • \$\begingroup\$ I second the suggestion to really familiarize yourself with LINQ. It's a huge time/effort saver. \$\endgroup\$ – John Stritenberger Sep 19 '17 at 23:05
  • \$\begingroup\$ Thanks, SnOrfus, although I did use LINQ in some places in the above code, but I agree, there is absolutely no reason not to use it extensively! \$\endgroup\$ – Maverick Sep 19 '17 at 23:17
  • 1
    \$\begingroup\$ Small note: your Integers method doesn't need for loop, just use Enumerable.Range(0, int.MaxValue). \$\endgroup\$ – Maxim Sep 20 '17 at 1:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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