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.

I'm pretty new to C# and have decided that I wanted to make a program that calculate the multiplication table of a selected number to practice on loops etc. I'm pretty satisfied, but I wondered if there is anything I can do to make the code better and more concise.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Multiplication
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Write a number and I'll calculate the multiplication table of the number");
            int userValue_number = int.Parse(Console.ReadLine());

            Console.WriteLine("How far do you want to go? E.g. 12, then {0} x 12 is the highest I will go ", userValue_number);            
            int userValue_length = int.Parse(Console.ReadLine());

            int n = userValue_number;
            int i = 1;
            while (n <= (userValue_number * userValue_length))
            {
                Console.WriteLine("{0} x {1} = {2}", i, userValue_number, n);
                n=n+userValue_number;
                i++;
            }
            Console.ReadLine();
        }
    }
}
share|improve this question

1 Answer 1

up vote 6 down vote accepted

int.Parse will throw an exception if input isn't an integer. Consider using int.TryParse instead.

You should verify that the user doesn't enter a negative integer for the 2nd argument.

Your while loop should be a for loop.

A multiplication isn't much more expensive than an addition, especially compared with the performance cost of calling Console.WriteLine; so I'd use multiplication instead if that makes the code clearer:

for (int i = 1; i <= userValue_length ; ++i)
{
    Console.WriteLine("{0} x {1} = {2}", i, userValue_number, i * userValue_number);
}

userValue_number is a strange mix of camelCase and using_underscores. camelCase is more conventional in C#. I think I'd rename those to chosenNumber and chosenTimes or something like that.

The final Console.ReadLine(); is ugly and uncessary. You probably use it for debugging; instead (to inspect output from a debugger session before the console window auto-closes) put a debugger breakpoint on the final curly brace of main.

Your message E.g. 12, then {0} x 12 is ... doesn't match your output {0} x {1} = ...; instead i should be the 2nd (not 1st) number in your output.

share|improve this answer
2  
Also, this version is not affected by infinite loops like OP's version (when userValue_number is 0) –  Josay Mar 1 at 22:49
1  
I'd upvote that if you post it as a separate answer (because I especially admire code reviews which find bugs). –  ChrisW Mar 1 at 22:51

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.