3
\$\begingroup\$

In the interview you are asked to build a tower in a variable input e.g. n=3 . The output should be as show below.

[
  '  *  ', 
  ' *** ', 
  '*****'
]

My solution is the following. Is this a good enough answer for the problem because I know people can solve it using Linq with less lines of code.

    public string[] PrintStairs(int n)
    {
        if (n == 1)
        {
            return new []{ " * " };
        }
       
        var result = new string[n];
        for (int row =0; row < n; row++)
        {
            var level = new StringBuilder();
            var midpoint = ((2 * n - 1) / 2);

            for (int col =0; col < 2 * n - 1; col++)
            {
                if((midpoint-row <= col) && (midpoint + row >= col))
                {
                    level.Append("*");
                }
                else
                {
                    level.Append(" ");
                } 
            }
            result[row] = string.Concat(new string(level.ToString()));
        }
        return result; 
    }

Linq Solution:

  public static string[] TowerBuilder(int nFloors)
  {
      return Enumerable.Range(1, nFloors).Select(i => string.Format("{0}{1}{0}", i == nFloors ? "" : new string(' ', nFloors - i), new string('*', 2 * i - 1))).ToArray();
  }
\$\endgroup\$
  • 3
    \$\begingroup\$ Personal opinion: If I were code reviewing this, I would ask for consistency in spacing (e.g. midpoint-row vs midpoint + row. \$\endgroup\$ – John Pavek Jun 30 at 23:40
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. Leave the concerns for the question body. \$\endgroup\$ – Mast Jul 1 at 6:04
  • \$\begingroup\$ (1) "Is this a good enough answer" isn't meaningfully answerable without understanding both your skill level and the expected skill level you're interviewing for, and any possible modifiers/priorities you've been given to solve this exercise. (2) "because I know people can solve it using Linq with less lines of code" Lines of code is not the main measure of quality. If anything, the LINQ example "cheats" by inlining a lot of different steps and ends up with compromised readability because of it. \$\endgroup\$ – Flater Jul 1 at 11:51
  • \$\begingroup\$ Jeff E has a very excellent answer but I would add that you are using var in a very lazy manner, one should only use var when the type can be easily inferred from the name of the variable or when dealing with Linq. \$\endgroup\$ – CaptianObvious Jul 1 at 12:21
  • \$\begingroup\$ I always use var is all my coding instead of the declaring the actually property type and its a coding standard within the company i work \$\endgroup\$ – Great Khan 2016 Jul 3 at 9:46
5
\$\begingroup\$

Is this a good enough answer?

This isn't meaningfully answerable without understanding both your skill level and the expected skill level you're interviewing for, and any possible modifiers/priorities you've been given to solve this exercise.

Because I know people can solve it using Linq with less lines of code

Line count is not the main measure of quality. If anything, the LINQ example "cheats" by inlining a lot of different steps and ends up with compromised readability because of it.


That being said, there are certain improvements to be made here. You've made things much more complex than they need to be. I'm going to make an educated guess here and say that you were shotgun coding and then did not clean up/refactor afterwards.

You would've been better off taking a step back and figuring out if you could calculate some things instead of iteratively needing to exploring them. If you had done so, you would've been able to break things down into three easy steps.

The main example of this is that you're adding every whitespace individually, which is not performant. It could've been calculated once, instead of iteratively adding them. A basic formula would be:

amount_of_blanks_on_one_side = tower_height - current_step_index - 1

Or if your steps are counted using 1-indexing (which I will actually use in this case:

amount_of_blanks_on_one_side = tower_height - current_step

Pick any tower size, and you'll see that this formula is correct for every step of every possible tower.

I'm not going to get into the specific code for this yet, but this already suggests that you have a clearly abstractable method here:

string stepString = GenerateStep(towerHeight, currentStep);

We've now split our logic into two separate steps:

  • How we generate a single step (GenerateStep)
  • How we combine these single steps into a tower

Let's focus on the second one now. It is assume that we already know the tower height, so let's work with a basic variable for now:

int towerHeight = 3;

Then, we're going to need a list of the steps (i.e. the number denoting each step. That is always going to be a list of numbers starting at 1 and ending at towerHeight, which can be easily generated using:

var steps = Enumerable.Range(1, towerHeight);

// steps: [ 1, 2, 3 ]

And now, we want to convert each step into the string we need it to be. Luckily, we already defined this as the GenerateStep method:

var tower = steps.Select(step => GenerateStep(towerHeight, step));

// tower: [ "  *  ", " *** ", "*****" ]

And that's all you need to do to get the tower you're looking for. Of course, we still need to look at how you generate the step string, but I want you to see how simple the rest of the logic really is.


Now, all we need to do is figure out how to generate a given step, based on the tower height and current step number. We don't need to worry about loops or towers anymore, we can focus solely on generating a single step. This will be wrapped in the method we already defined:

public string GenerateStep(int towerHeight, int currentStep)
{
    // ...
}

A step is always of the form:

[X amount of blanks][Y amount of asterisks][X amount of blanks]

So we really just need to figure out what X (which I'll call the padding) and Y (which I'll call the width of the tower) are.

We actually already established how to find the padding (X):

amount_of_blanks_on_one_side = tower_height - current_step

Or, in code:

int paddingSize = towerHeight - currentStep;

The width of the tower is slightly more complex. Let's look at some numbers to see if we spot the formula

STEP | WIDTH
------------
  1  |  1
  2  |  3
  3  |  5
  4  |  7
  5  |  9

Depending on how you look at it, you should quickly figure out that the width is always one less than the double of the step number, i.e.:

int towerWidth = 2 * currentStep - 1;

So now we know our width and padding. The only thing that's left is how to generate a string of a given length with a repeating character. There is actually a string constructor for that:

var myString = new string(myCharacter, amountOfCharacters);

Therefore, we can generate the appropriate strings:

string padding   = new string(' ', paddingSize);
string towerPart = new string('*', towerWidth);

And then we put these string together:

string result = $"{padding}{towerPart}{padding}";

And that's it.


Putting it all together:

public IEnumerable<string> GenerateTower(int towerHeight)
{
    var steps = Enumerable.Range(1, towerHeight);

    var tower = steps.Select(step => GenerateStep(towerHeight, step));

    return tower;
}

public string GenerateStep(int towerHeight, int currentStep)
{
    int paddingSize = towerHeight - currentStep;
    int towerWidth  = 2 * currentStep - 1;

    string padding   = new string(' ', paddingSize);
    string towerPart = new string('*', towerWidth);

    return $"{padding}{towerPart}{padding}";
}

If you examine the steps closely, this actually mirrors the LINQ example you provided. But the LINQ example is horribly unreadable, which is why I opted for a bit more verbosity to ensure that the code remains clean and readable.

Could you optimize this further? Yes, and I'm sure this has already been golfed. But further condensing the code is only going to detract from the readability, which is why you're better off not optimizing any further (unless you are dealing with very specific performance constraints).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ When you do the calculation for int paddingSize = towerHeight - currentStep. If(n =3) for first loop with have a padding size of 2. So how does that work? I know the answer is correct \$\endgroup\$ – Great Khan 2016 Jul 3 at 10:17
  • \$\begingroup\$ @GreatKhan2016: It helps to count from the ground up (= the end of the array). The last element should have 0 padding. The one above it should have 1 padding. The one above it should have 2 padding. And so on... You'll notice that for any tower with n steps, the paddings start at n-1 and decreases all the way down to 0. For for n=3 that's padding sizes [2,1,0], for n=5 that's padding sizes [4,3,2,1,0] and for n=100 that would be padding sizes [99, 98, 97, ..., 2, 1, 0]. \$\endgroup\$ – Flater Jul 3 at 11:35
  • \$\begingroup\$ @GreatKhan2016: If you're unsure how to find a formula for calculating a number, it helps to write down a bunch of examples and see if you can spot a pattern. If you can guess the next number in the sequence, that means there is a pattern to the sequence and therefore an underlying formula that expresses that pattern. You can see in the answer that I did this when we were trying to figure out how to calculate the width of the tower. You can use the exact same approach when you want to calculate the padding size. \$\endgroup\$ – Flater Jul 3 at 11:43
20
\$\begingroup\$

How did you determine the output ' # ' for the case n==1? I'd have expected it to just be '*'. If you're unclear about the specification for code you're writing in an interview, it's best to either ask for clarification or document your assumptions -- it's possible they deliberately handed you an unclear spec and are testing how you handle that.

If we take the interviewer request literally, you're still missing the [ ... ] formatting.

Naming

public string[] PrintStairs(int n)

This method is named as a verb indicating it's useful for its side effect, but it returns a value indicating it's useful for its return value. You should design APIs for one or the other but not both. Since your method is useful for its return value, a more appropriate name might be GenerateStairs or GetStairs.

Use more descriptive parameter names. numberOfStairs instead of n for example.

You use two different names, row and level, for different manifestations of the same concept: a step in your staircase. I'd unify these and call them rowIndex and rowBuilder respectively, or maybe stepIndex and stepBuilder.

Input validation

Your method crashes for negative values of n. If negative numbers are not valid inputs to the method, that fact should be documented by a method header and by throwing an ArgumentOutOfRangeException.

String APIs

result[row] = string.Concat(new string(level.ToString()));

This is a very complicated way of saying result[row] = level.ToString()! string.Concat is an API for concatenating strings, but you are only giving it one string which does nothing but give you the same string out again.

Explicit string constructors are intended to be used for creating strings from individual characters or from unmanaged code; here you're just giving it another string, which also does nothing but give you the same string out again.

General behaviour

var midpoint = ((2 * n - 1) / 2);

You are doing integer division here. Double-check your math because this expression simplifies to n - 1 for positive n -- were you intending for something more elaborate to happen with this calculation? A reader might wonder if there's a bug here.

n doesn't change inside the loop, so this whole line can be moved outside of it.

General formatting

Your spacing is inconsistent:

for (int row =0; row < n; row++)

if((midpoint-row <= col) && (midpoint + row >= col))

for (int col =0; col < 2 * n - 1; col++)

LINQ

Having everything on one line isn't necessarily a good thing. The LINQ solution is impenetrable when written like that. It deserves to be on multiple lines anyway for the sake of being able to read it properly:

return Enumerable.Range(1, nFloors)
    .Select(i => string.Format("{0}{1}{0}",
        i == nFloors
            ? ""
            : new string(' ', nFloors - i),
        new string('*', 2 * i - 1)))
    .ToArray();

Yes, it's technically LINQ, but the guts of that Select is what's doing all the heavy lifting.

The LINQ solution doesn't give the same output for n==1, which again begs the question where you got the idea that it should output ' # ' in the previous solution.

A more LINQ-y solution would have you build up the building blocks needed to write code that really is shorter. LINQ is all about operations on sequences; the thing that throws a wrench in any LINQ solution here is the fact that the output depends on how many things are in the sequence (the amount of space you use to pad the stairs depends on how many stairs there are), so you're probably going to have to enumerate sequences more than once.

Here's my LINQ-y take: start with a simple enumerator that gives you unformatted steps, from which you can take any arbitrary number of them:

private string GenerateStep(int stepIndex)
{
    if (stepIndex < 1)
        throw new ArgumentOutOfRangeException(nameof(stepIndex));
    return new string('*', stepIndex * 2 - 1);
}

private IEnumerable<string> EnumerateSteps()
{
    for (int stepIndex = 0; stepIndex < int.MaxValue; stepIndex++)
    {
        yield return GenerateStep(stepIndex);
    }
}

Now I can say EnumerateSteps().Take(3) to get:

[
  '*',
  '***',
  '*****'
]

Then define a method that takes a sequence of strings and centers them:

static class StringExtensions
{
    private static string CenterFormatted(string original, int totalWidth)
    {
        if (original == null)
            throw new ArgumentNullException(nameof(original));
        if (totalWidth < original.Length)
            throw new ArgumentOutOfRangeException(nameof(totalWidth));

        int paddingNeededOnLeft = (totalWidth - original.Length) / 2;
        int paddingNeededOnRight = totalWidth - original.Length - paddingNeededOnLeft;

        return String.Concat(
            new string(' ', paddingNeededOnLeft),
            original,
            new string(' ', paddingNeededOnRight));
    }

    public static IEnumerable<string> CenterFormatted(this IEnumerable<string> sequence)
    {
        // Exercise: how will you check for a null input sequence here? If you check it here and throw ArgumentNullException, will it actually throw when you call CenterFormatted?
        int largestStringLength = sequence.Max(s => s.Length);
        foreach (string original in sequence)
        {
            yield return CenterFormatted(original, largestStringLength);
        }
    }
}

Now you have two orthogonal and reusable things: a thing that generates steps and a thing that centers sequences of strings. You can compose these quite concisely in a LINQ statement to solve the original problem:

EnumerateSteps().Take(3).CenterFormatted().ToArray()

which gives me

[
  '  *  ', 
  ' *** ', 
  '*****'
]
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I made a mistake with "# ' for the case n==1". Thanks for the reply. \$\endgroup\$ – Great Khan 2016 Jul 3 at 9:53
1
\$\begingroup\$

The first things that strike me:

  • Where is the documentation?
  • How do I know what the code is intended to do (which may be different from what it does)?
  • Why are there no comments at all in/near the code?

As such, the code is not maintainable, neither by you in 6 months or some poor maintenance programmer who gets to fix/update the program. (Always program assuming the maintenance programmer who will have to keep your code alive is a homicidal maniac with a chainsaw and knows where you live.)

Given a number i, this will return an array of i elements. The elements are strings that, when printed/displayed with a monospaced font, will show a triangle. With an input of 3, an array containing these 3 elements is returned:

'  *  '
' *** '
'*****' 

There are also no tests, but that may be justified if it is just an example. (Input validation etc. etc. etc. have been mentioned by others.)

Unless there is a really, really good reason, you need your code to be (in this order):

  1. correct: getting the wrong answers makes the code utterly useless.
  2. maintainable: a new programmer (or you in 6 months) must understand this.
  3. sufficiently fast while not being an undue burden on memory, I/O, storage space etc.

Why is speed/memory/... not placed higher? Because usually these limitations can be solved by throwing money at it, whereas a non-maintainable program is a millstone round the neck.

LoC is a terrible measurement.
Performance
calculated
by
Lines
of
Code
just
causes
more
line-
breaks.
(See how productive I have been?)

Elegant code is maintainable: succinct, minimum boilerplate code, idiomatic use of the language.

There are code formatters for C# out there: use them! Tweak then to fit the style guide used --- if necessary create a style guide.

Consistency is especially important when there are/will be more than one single programmer.

Your choice of name is ... interesting. Nothing is actually printed. Even when the result is printed, you will get no stairs. It'll be a triangle, a pyramid, maybe even a "tower". Good names provide a very useful abstractions.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Sorry it is a coding challenge and i want to know if i spend 20 mins writing the code if it was good enough \$\endgroup\$ – Great Khan 2016 Jul 3 at 9:57

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.