-2
public static void Main()
{
    int i, j;

    Console.Write("    ");

    for (i = 1; i < 10; i++)
        Console.Write(String.Format("{0,3:d}", i));

    Console.WriteLine();

    for (i = 0; i < 32; i++)
        Console.Write("=");
    
    Console.WriteLine();

    for (i = 1; i < 10; i++)
    {
        Console.Write(i + " | ");

        for (j = 1; j < 10; j++)
            Console.Write(String.Format("{0,3:d}", i * j));

        Console.WriteLine();
    }

    Console.ReadKey();
}

enter image description here

Some person did show me this piece of code and told that

Any programmer, systems engineer, or systems mathematician who has seriously studied C# should know the answer.

I don't see anything too wrong with it.

  1. It has table limits hardcoded (10), but the table itself is for 9 numbers only, so this is not a crime.

  2. Doing i + " | " is not very nice for performance and memory usage, but this is such trivia.

The person does not respond to me what's was wrong with this code, I can't sleep now thinking about it!

TylerH
  • 20,799
  • 66
  • 75
  • 101
Kosmo零
  • 4,001
  • 9
  • 45
  • 88
  • Better to declare the loop counter variables in the loop to keep the scope as narrow as possible and always use braces even for one liners. – Retired Ninja Jul 14 '22 at 08:55
  • @RetiredNinja - I never knew about this. Can you explain why? Especially about the braces. This is so awesome that I can skip braces for single line, I always use this feature. – Kosmo零 Jul 14 '22 at 08:57
  • Personally I prefer to use braces around single lines as it prevents errors when a single line block becomes multiple lines and forgetting to add the necessary braces. I also find it easier to read. – phuzi Jul 14 '22 at 08:58
  • 2
    The problem with skipping braces is that it's so easy to forget to add them in when you suddenly need multiple statements to be executed for each loop iteration, or if the condition of the if statement is met, etc. – ProgrammingLlama Jul 14 '22 at 08:59
  • There's no need for "global" loop variables. Just declare the loop variable in the for loop initialisation `for (int i = 1; i < 10; i++)` etc. This way there's no leak and potential for bugs caused by not initialising the variable value correctly. – phuzi Jul 14 '22 at 09:00
  • @phuzi - but doesn't it add extra time to create those variables? Like now they exist from the beginning and the memory for them was reserved only once. The thing you telling me about will end in a need to reserve memory for them 4 times. – Kosmo零 Jul 14 '22 at 09:02
  • 2
    Yes but the difference is miniscule. In general it's better to worry more about maintainability and prevention of bugs and not worry too much about performance if it makes the code less maintainable or has the potential to introduce bugs until it really matters. Check out [this article about the evils of premature optimisation](https://stackify.com/premature-optimization-evil/). – phuzi Jul 14 '22 at 09:04
  • 1
    Can't see too much wrong with it apart from minor style/formatting issues and the global loop variables which in this example would not make much difference to performance and might actually improve it due to less allocations. Don't think that there is anything major objectively wrong with it. – YungDeiza Jul 14 '22 at 09:06
  • 1
    Just a guess, but is he alluding to the fact that you're calculating most of the vales twice? So you have calculated both 2x5 and 5x2, for example. – jonsca Jul 14 '22 at 09:10
  • Skip this job because this one tends to make things mysterious – Xiang Wei Huang Jul 14 '22 at 09:12
  • 1
    You can replace the for loop of printing `=` with `Console.Write(new string('=', 32));` – Chetan Jul 14 '22 at 09:14
  • @jonsca - This can be true, but... I can't even imagine how to reuse multiplication result easy in this case. – Kosmo零 Jul 14 '22 at 09:21
  • 2
    For the question around variables creating performances ... [This might be handy.](https://stackoverflow.com/questions/1884906/declaring-a-variable-inside-or-outside-an-foreach-loop-which-is-faster-better). But yeah, that one doesn't seems like C# coding style. Usually smallest scope declaration is better. Declaring `i` and `j` out of the loops also causes issues too: you can't use `i` and `j` as variable names later in that scope. `i` and `j`'s last values also lingers around until program is ended. – Xiang Wei Huang Jul 14 '22 at 09:26
  • For the reusing - yes it's tricky to think, but that's where space of optimization is. By the way, as this question post doesn't have an actual answer, it doesn't feel like a post fit for stackoverflow. – Xiang Wei Huang Jul 14 '22 at 09:28
  • @XiangWeiHuang - Thank you. I will delete it then (or not, since I got a least single answer). – Kosmo零 Jul 14 '22 at 09:29
  • 1
    It's a matter of trading off memory space (an array perhaps) for processing speed. For such a small task, though, the gains are trivial and this interviewer probably thinks too highly of himself! – jonsca Jul 14 '22 at 09:40

2 Answers2

3

This question might be better asked here: Code review

  • Correctness: it behaves correctly
  • Readability: simple program, not too bad
  • Efficiency: not too bad either
  • Flexibility: poor - I suggest implementing a more generalized method which takes a table width/height parameter
  • Other: String-handling is a bit archaic (can use string interpolation)

Beyond these few comments I can see a couple of reasons one might argue religiously, i.e. not about right and wrong but about "best practice" (dogma) - like use of braces

AlanK
  • 1,827
  • 13
  • 16
  • Thank you. I didn't even knew about some `Code review` thing. I always find `stackoverflow` in `Google`, so though this is the place for everything. – Kosmo零 Jul 14 '22 at 09:46
  • 1
    @Kosmo零 You are welcome. You can link to other Stack Exchange sites - including Code Review - from the icon on the far right of the stackoverflow header / title bar. – AlanK Jul 14 '22 at 10:02
1

I tried to minimize and optimize the code and add support for determining the range (min/max) of the table to be printed so that it calculates the necessary space for longer numbers.

Many programmers are allergic to the missing parentheses for single line subblocks (conditions or loops), but I'm more comfortable without them, the code is not as long and so much more readable for me. But it's more about habit. Maybe they have it as a mandatory convention in that company, but then I would have other comments on code optimization than this irrelevance.

In Python, they also just indent instead of bracket and nobody there minds.

using System;
using System.Linq;


MultipleTable(1, 9);

void MultipleTable(int min, int max)
{
    int minDigits = max.ToString().Length,
        maxDigits = (max*max).ToString().Length;

    Console.Write(" | ".PadLeft(minDigits+3));
    Console.WriteLine(String.Join(" ", 
        Enumerable.Range(min, max-min+1)
        .Select(x => x.ToString().PadLeft(maxDigits))));

    Console.WriteLine("".PadRight((max-min+1)*(maxDigits+1)+minDigits+3, '='));

    for (int i = min; i <= max; i++)
    {
        Console.Write(i.ToString().PadLeft(minDigits) + " |");
        for (int j = min; j <= max; j++)
            Console.Write((i*j).ToString().PadLeft(maxDigits+1));
        Console.WriteLine();
    }

}

You can try it in SharpLab.

Petr Voborník
  • 1,249
  • 1
  • 14
  • 11
  • Have you compared this to the original code in terms of speed? I'm almost certain that this will be slower. – YungDeiza Jul 14 '22 at 09:41
  • 1
    Tested both and this is slower and less readable than the original in my opinion – YungDeiza Jul 14 '22 at 09:42
  • Thank you. Your code is cool. It became hard to understand (without originally knowing what it is used for), but it is generic now. – Kosmo零 Jul 14 '22 at 09:47
  • Yes, it can be slower, because it is universal for more variants of wrote range. This code automatically counts if it needs one, two or more spaces for number padding. That's also why I can't use interpolation with constants and speed things up overall. – Petr Voborník Jul 14 '22 at 09:56