Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

One of the books I'm currently reading to learn C# asked me to write a method that takes an integer between 1 and 99999 and displays each number in that integer in a sequence, separating each digit by 2 spaces.

For example: the int 87564 would be written to the console as 8 7 5 6 4.

This is simple enough to do using the input as a string, converting it to a char array, or looping over the string with a foreach and printing out a formatted string for each character.

For fun though and mostly to challenge myself, I like to work out the problems as they are intended for someone just learning the concepts for the first time. The chapter was about methods and briefly introduced recursion. It's clearly the author's intent that you solve this using division and modulus operations to pick off each digit and then write them out.

So there really were limited options in terms of solving this with the material you have learned to this point in the book. You could pick off each digit and store it as it's own variable, then later write them out in order since you know the range of integers.

I decided to make the method more useful by really allowing any non-negative integer and my approach involved recursion. I'm not really experienced using recursion so I'd like to get some feedback on my implementation to see what I could have done better.

public class Program
{
    static void Main()
    {
        // Get User Input From Console
        // Validate and parse as int input
        DisplayDigits(input);
    }

    static void DisplayDigits(int value)
    {
        if (value < 10)
        {
            Console.Write("{0}  ", value);
            return;
        }
        DisplayDigits(value / 10);
        Console.Write("{0}  ", value % 10);
    }
}

Obviously, this is not meant to be production level code, I'm solving the problem in the textbook and then throwing the code away. It appears to be working for all non-negative numbers I've tried. I even wrote an overload that takes a ulong and passed it UInt64.MaxValue and it printed everything fine. I can't help but feel like maybe it could be better in some way. Any criticism, suggestions, links to more reading material would be appreciated.

share|improve this question
1  
Welcome to CR! This small snippet makes your post very focused on the specific problem you're trying to solve (which isn't bad in itself). You'd probably get a more thorough review if you included the entire code (I presume DisplayDigits is static because it's called from static void Main()?). –  Mat's Mug Feb 7 at 18:30
1  
Your code displays the digits in reverse order. Not in the same order as they appera in your example. –  Andris Feb 8 at 13:54
add comment

4 Answers

up vote 7 down vote accepted

I'm not too worried about separation of concerns for such a simple problem. You should try to eliminate a Console.Write() repetitive call, though.

static void DisplayDigits(int value)
{
    if (value >= 10)
    {
        DisplayDigits(value / 10);
    }
    Console.Write("{0}  ", value % 10);
}
share|improve this answer
    
This is pretty much the type of feedback I was looking for. I understand the usefulness of separation of concerns and why writing code that way is important, this was more about picking out little details (too many Console.Write calls). –  Gary Justin Feb 9 at 3:29
add comment

Your method is lying. Not only it's displaying the digits, performing every Console.Write operation that needs to happen, it's also performing the "digit-splitting" logic.

It's more work than what its name says. For a simple coding exercise it's without consequences, but in larger projects if this is a coding habit you have, it can mean much bigger problems.

Separation of Concerns

Let's see what needs to happen here.

  • Get a valid user input.
  • Determine what the digits are.
  • Come up with a string to output.

You haven't included how you're addressing the first concern, so I won't cover it here.

Ideally, the result of determining what the digits are should be just that: some IEnumerable<int> that contains all the digits you want to display. I would make it so that the recursive function that contains the logic for that, works with an IList<int> and adds each digit it finds (instead of outputting it directly to the console).

Then, when your recursive logic completes, you have an IEnumerable<int> to iterate through. Right? Maybe not. string has methods, such as Join() that can make your "digit separator" string very explicit - if you're using .net 4+ you can use the Join<T>(string, IEnumerable<T>) overload:

var input = GetValidUserInput();
if (string.IsNullOrEmpty(input))
{
    return;
}

var digits = SeparateTheDigits(input);

var separator = "  ";
var result = string.Join(separator, digits);

Console.WriteLine(result);

As a result of separating the concerns, you now only write to the console once... and you haven't hard-coded your digit separator into a format string.

share|improve this answer
1  
Perhaps GetValidUserInput() is going to validate that the input is a valid number - in that case you'll probably want the method to return an int, so SeparateTheDigits can work off an int instead of directly with the user's input. –  Mat's Mug Feb 7 at 19:08
    
I like to keep my logic on the same level of complexity and if you want to do that you would move the separator/result/write into a separate method. –  helgeheldre Feb 7 at 19:14
3  
I guess this question is more about maximizing and working within the restrictions of the textbook. At this point, the author hasn't talked about arrays or working with the String class. At the end of the day it's throwaway code from a textbook exercise, but for the sake of mastering the simple things, I thought I'd see what else could be done without using arrays or treating it as a string. The author assumes you will get the input from Console.ReadLine and use Convert.ToInt32. I think for this particular case, all I could really do is have GetDigits return a string instead. –  Gary Justin Feb 7 at 19:29
add comment

I strongly agree with lol.upvote answer, regarding the separation of concerns. I Think you should have a method to get the chars and then present them to the user, because you may need know what chars of a number are later. And so I made all the major approaches that you may have when implementing a GetDigits. A iterative, recursive and a straightforward way:

public static char[] GetDigitsRecursive(int value){
    int length = value.ToString().Length;
    return GetDigitsRecursiveAux(value, new char[length], length-1);
}

private static char[] GetDigitsRecursiveAux(int value, char[] digits, int idx){
    digits[idx] = (char)(value % 10 + '0');
    if(value <= 9){
        return digits;
    }
    return GetDigitsRecursiveAux(value/10, digits, idx-1);
}

public static char[] GetDigitsIterative(int value){
    char[] digits = new char[value.ToString().Length];
    for(int i = digits.Length-1; i >= 0; --i){
        digits[i] = (char)(value %10 + '0');
        value = value / 10;
    }
    return digits;
}

public static char[] GetDigitsSimplistic(int value){
    return value.ToString().ToCharArray();
}
share|improve this answer
2  
Nice! lol.upvoted! –  Mat's Mug Feb 7 at 19:18
2  
It feels weird to call ToString(), which almost does what is needed here and then throw away its result except for the length. –  svick Feb 18 at 16:17
    
It is quite true, that's why I pointed out the third way (and that's the one I would use). The other ones are exercises only. But if you would rather do a function to count the digits then it's your call... –  Bruno Costa Feb 18 at 17:01
add comment

Here is a more functional approach using recursion and returns back the formatted string rather than writing to Console.

static 
string
FormatDigits(
    int                                 value)
{
    return
        value < 10
            ? value.ToString()
            : String.Format(
                        "{0} {1}",
                        FormatDigits(value / 10),
                        value % 10);

}

Update: Some explanation for those unfamiliar with recursion or find this code difficult to digest. Recursion provides a way to reduce the problem into a set of smaller problems and simpler solutions.

There are 2 cases here.

  1. If the value < 10 (exactly one digit)
    Simply convert the digit to a string, (value.ToString()) and return it
  2. If the value >= 10 (more than one digit)
    The value % 10 will give the last digit, use this value as the last character of the result (with a space prefix) the " {1}" part of the string format.
    The problem is now reduced to doing the same thing all over again, this time with a value without the last digit (value / 10) and placing the result in the "{0}" part of the format string.

Update 2: Don't like the string format or find it difficult to understand? Here is another refactor.

static string FormatDigits(int value)
{
    return value < 10
            ? value.ToString()
            : FormatDigits(value / 10) + " " + FormatDigits(value % 10);
}
share|improve this answer
1  
Hmm. This is Code Review, not Code Golf. I wouldn't want to maintain c# code written like this! –  Mat's Mug Feb 8 at 3:13
    
Downvote removed. –  Mat's Mug Feb 9 at 0:26
1  
Upvote applied. The one-liner should be the signature ;) –  Mat's Mug Feb 9 at 4:13
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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