Climbing the stairs

Question

A quick description on the problem I solved on codewars.com but remember I just want a small part of my solution refactored (just the for-loop):

Description

Vasya wants to climb up a stair of certain amount of steps (Input parameter 1). There are 2 simple rules that he has to stick to.

Vasya can climb 1 or 2 steps at each move. Vasya wants the number of moves to be a multiple of a certain integer. (Input parameter 2). Task:

What is the MINIMAL number of moves making him climb to the top of the stairs that satisfies his conditions?

Input

Number of stairs: 0 < N ≤ 10000 ; Integer to be multiplied : 1 < M ≤ 10; Output

Return a single integer - the minimal number of moves being a multiple of M; If there is no way he can climb satisfying condition return - 1 instead.

This is my C# method and I only want to refactor the for-loop on line 13:

private static int NumberOfSteps(int n , int m)
    {
        Console.WriteLine(n + "    " + m);
        if (m <= 1 || n <= 1) { return -1; }
        if (n == 21) { return 20; }

        var TwoSteps = Math.Ceiling(((double)n / 2)) % m != 0 ? ((n / 2) - 1) : (n / 2);
        List<int> query = Enumerable.Range(1, TwoSteps).Select(x => x = 2).ToList();
        List<int> Onesteps = Enumerable.Range(1, n - query.Sum()).Select(x => x = 1).ToList();
        query.AddRange(Onesteps);
        if (query.Count() % m == 0) { return query.Count(); }

        for (var count = 0; count <= query.Count() - 1; count++)

        {
            if (query[count] == 2)
            {
                query.RemoveAt(count);
                query.AddRange(new List<int>() { 1, 1 });
                if (query.Count() % m == 0) { Console.WriteLine(query.Count()); return query.Count(); }
            }
            if (query.Count() % m == 0) { return query.Count(); }
        }
        Console.WriteLine(-1);
        return -1;
    }

This is part of my code I would like to know how I could refactor it into a Linq Method Syntax statement:

   for (var count = 0; count <= query.Count() - 1; count++)

    {
        if (query[count] == 2)
        {
            query.RemoveAt(count);
            query.AddRange(new List<int>() { 1, 1 });
            if (query.Count() % m == 0) { Console.WriteLine(query.Count()); return query.Count(); }
        }
        if (query.Count() % m == 0) { return query.Count(); }
    }

Answer 1

The code below is my C# Method and I only want to refactor the for-loop on line 13 I think.

Sorry but you won't only get this, because your code has a large number of problems.

Let us begin at the top of your code and review it to the bottom.

Return a single integer - the minimal number of moves being a multiple of M; If there is no way he can climb satisfying condition return - 1 instead.

Why bothering with printing to the Console if you only need to return an integer ?
Your method should have a clear responsibility which is returning the minimal numbers of moves. Printing some values to the Console doesn't belong here.

---

The description of the problem clearly states that the number of stairs are in the range 0 < N ≤ 10000.
So if the passed in parameter(s) doesn't fit into the allowed range you should throw an ArgumentOutOfRange exception instead of returning -1.

The if (n == 21) { return 20; } "validation" will lead to wrong results as well.

---

This

var TwoSteps = Math.Ceiling(((double)n / 2)) % m != 0 ? ((n / 2) - 1) : (n / 2);  

is hurting my eyes because it is almost unreadable. In addition , if we look a little bit down the code we see a OneSteps variable which is of a different type but one could assume that it is the same because of the name. So we should change the name here to amountOfTwoSteps and use the proper casing (camelCase) for the name.

This is doing the same

var amountOfTwoSteps = n / 2;

if (amountOfTwoSteps % m != 0)
{
    amountOfTwoSteps--;
} 

but in a cleaner way.

---

You should be consistent in the style you use. If you use the var type then you should use it where ever the type of a variable is obvious from the right hand side of the assignment.

List<int> query = Enumerable.Range(1, TwoSteps).Select(x => x = 2).ToList();  

but it is a very strange way to get a list of the same number (here: 2). You could use Enumerable.Repeat() instead like so

var query = Enumerable.Repeat(2, amountOfTwoSteps).ToList();  

This

List<int> Onesteps = Enumerable.Range(1, n - query.Sum()).Select(x => x = 1).ToList();  

has another problem, because it iterates over all of the items of query by calling Sum() but you know that query has amountOfTwoSteps items with each item equals to 2. So let us change query.Sum() to amountOfTwoSteps * 2.

You don't need to call ToList() on the enumerable because the AddRange() method of the List<T> will take an IEnumerable<T> as a parameter. You don't really need the OneSteps variable and the code could be changed like so

query.AddRange(Enumerable.Repeat(1, n - (amountOfTwoSteps * 2)));

---

You are using the Count() method very often, which involves for a List<T> a null check, a soft cast via as to ICollection<T> and again a null check` each time.

From the reference source

    public static int Count<TSource>(this IEnumerable<TSource> source) {
        if (source == null) throw Error.ArgumentNull("source");
        ICollection<TSource> collectionoft = source as ICollection<TSource>;
        if (collectionoft != null) return collectionoft.Count;
        ....
        ....  

If you know that the type you use is an implementation of an ICollection<T>, like the List<T> you should consider to just access the Count property.

---

if (query[count] == 2)
{
    query.RemoveAt(count);
    query.AddRange(new List<int>() { 1, 1 });
    if (query.Count() % m == 0) { Console.WriteLine(query.Count()); return query.Count(); }
}
if (query.Count() % m == 0) { return query.Count(); }

If we look at the body of your for loop, we can see that you have some duplicated code which can be removed. I don't see a reason to remove an item of the List, create a new List<T> and use AddRange to add this List to query every time. This could be simplified by setting the value of the current item to 1 and adding a 1 to the list like so

if (query[count] == 2)
{
    query[count] = 1;
    query.Add(1);
}
if (query.Count % m == 0) { return query.Count; }

---

Putting all together will lead to

private static int NumberOfSteps(int n, int m)
{
    if (m <= 1 || m > 10) { throw new ArgumentOutOfRangeException("m"); }
    if (n < 1 || n > 10000) { throw new ArgumentOutOfRangeException("n"); }

    var amountOfTwoSteps = n / 2;

    if (amountOfTwoSteps % m != 0)
    {
        amountOfTwoSteps--;
    }

    var query = Enumerable.Repeat(2, amountOfTwoSteps).ToList();
    query.AddRange(Enumerable.Repeat(1, n - (amountOfTwoSteps * 2)));

    if (query.Count % m == 0) { return query.Count; }

    for (var count = 0; count <= query.Count - 1; count++)
    {
        if (query[count] == 2)
        {
            query[count] = 1;
            query.Add(1);
        }
        if (query.Count % m == 0) { return query.Count; }
    }

    return -1;
}

which still has some magic numbers in it which should be extracted to some meaningful named constants which is for you to do.