I am stuck on the iterator/conditional section of my code, it does not complete the iteration as expected.

The routine accepts a random non negative integer and must return a rearranged integer where the leftmost digit is the greatest digit of the input integer and the rightmost digit is the smallest.

The code is as follows:

 def descending_order(n)
    new = n.digits
    result=[]
    for i in new
      if i=new.max() then new.delete(i) && result.push(i)  
      end
    end
    return result.join().to_i 
  end

The sample input is as follows:

descending_order(6022563)

A sample of the erroneus result I get is as follows:

descending_order(6022563) should equal 6653220 - Expected: 6653220, instead got: 653

CodePudding user response：

This doesn’t address WHY you’re having a problem with your code, but here’s an alternative solution:

def descending_order(n)
  n.digits.sort.reverse.join.to_i
end
descending_order(6022563)
#=>  6653220

CodePudding user response：

If you want to figure out what's going in your code, one of the best ways to do that, is to simply execute it step-by-step with pen and paper, keeping track of the current value of all variables in your code. So, let's do that.

However, before we start stepping through the code, let's first see if we can simplify it without changing how it works.

Firstly, the return is redundant. In Ruby, the return value of a block of code is the value of the last expression that was evaluated in that block. So, we can just remove the return.

Secondly, for … in is equivalent (not exactly, but the differences don't matter in this particular case) to #each, so your code could also be written like this:

def descending_order(n)
  new = n.digits
  result = []

  new.each do |i|
    if i = new.max
      new.delete(i) && result.push(i)
    end
  end

  result.join.to_i 
end

Next, let's look at this bit:

new.delete(i) && result.push(i)

This will delete all occurrences of i from new, and also test whether the return value of new.delete(i) is trueish. If and only if the value is trueish, result.push(i) will be executed.

However, new.delete(i) will only return a falseish value (namely nil) if the value i was not found in new. But, we just assigned i to be the maximum value in new, so we know it always exists and therefore, new.delete(i) will always return i and never nil. This means result.push(i) will always be executed and the conditional is not doing anything, we can just remove it:

new.delete(i)
result.push(i)

Now, let's look at the conditional:

if i = new.max

This assigns the maximum value of the new array to i and also tests whether that value is trueish. In other words, it is equivalent to

i = new.max

if i

In Ruby, the only two values that are falseish are false and nil, so the only way this conditional can fail is if the maximum value of the new array is either false or nil. Since we know that we created the array from the digits of a number, we know that it only contains integers, not nil nor false. Enumerable#max also returns nil if the array is empty, so, the only way this can fail is if the array is empty. In other words, it is equivalent to

i = new.max

if !new.empty?

or

i = new.max

unless new.empty?

However, we also know that we are in the middle of iterating over new, so it cannot possibly be empty! If it were empty, the iteration would be executed 0 times, i.e. we would not be hitting this conditional at all.

Therefore, the conditional will always be true and can just be removed:

def descending_order(n)
  new = n.digits
  result = []

  new.each do |i|
    i = new.max

    new.delete(i)
    result.push(i)
  end

  result.join.to_i 
end

Lastly, let's look at the iteration variable i. It will get assigned the current element of the iteration, but we immediately re-assign it on the first line of the block without ever looking at its value. So, it is actually not used as an iteration variable at all, it is simply a local variable and we can remove it from the block:

def descending_order(n)
  new = n.digits
  result = []

  new.each do
    i = new.max

    new.delete(i)
    result.push(i)
  end

  result.join.to_i 
end

With this simplified code in place, we are now going to look at each step of each iteration of the loop separately.

We start off with the following (let's imagine we are just before the start of the first iteration):

Variable	Value
new	[3, 6, 5, 2, 2, 0, 6]
result	[]
i

But, there is actually another, hidden, variable as well: the pointer to the current index. Remember, we are in the middle of iterating over new and each must internally somehow keep track of where we are. So, even though we don't know how each works internally, we can assume that it needs to somehow, somewhere, remember where we are. So, we have another piece of state to keep track of: the current index of the iteration.

Variable	Value
new	[3, 6, 5, 2, 2, 0, 6]
result	[]
i
index

Alright, let's look at the first step of the first iteration, which is

i = new.max

Variable	Value
new	[3, 6, 5, 2, 2, 0, 6]
result	[]
i	6
index	0: [→ 3 ←, 6, 5, 2, 2, 0, 6]

The next step is

new.delete(i)

which deletes all occurrences of i from new, i.e. it deletes all occurrences of 6 from [3, 6, 5, 2, 2, 0, 6]. This leaves us with [3, 5, 2, 2, 0], but what is also important is that each doesn't know what we are doing, that we are mutating the array. Therefore, the pointer will still stay at position 0, it will not move.

Variable	Value
new	[3, 5, 2, 2, 0]
result	[]
i	6
index	0: [→ 3 ←, 5, 2, 2, 0]

The next step is

result.push(i)

which appends i to the end of the result array:

Variable	Value
new	[3, 5, 2, 2, 0]
result	[6]
i	6
index	0: [→ 3 ←, 5, 2, 2, 0]

And that's it for the first iteration!

Let's look now at the second iteration. The first thing that is going to happen is that each internally increments its counter or moves its pointer, or however each is implemented internally. Again, we don't know how each is implemented internally, but logic dictates that it must somehow keep track of where we are in the iteration, and we can also assume that before the next iteration, it will somehow need to move this.

So, at the beginning of the iteration, the situation now looks like this:

Variable	Value
new	[3, 5, 2, 2, 0]
result	[6]
i
index	1: [3, → 5 ←, 2, 2, 0]

Alright, let's look at the first step of the second iteration:

i = new.max

We again assign i to the maximum value of new, which is now 5:

Variable	Value
new	[3, 5, 2, 2, 0]
result	[6]
i	5
index	1: [3, → 5 ←, 2, 2, 0]

Next step is

new.delete(i)

which deletes all occurrences of 5 from [3, 5, 2, 2, 0], which leaves us with [3, 2, 2, 0]. But remember what we also said above: each doesn't know what we are doing, that we are mutating the array. Therefore, the pointer will still stay at position 1, it will not move. But the item that is at position 1 (which is the number 5) will be deleted! That means, all elements after the 5, i.e. all elements after index 1 now get shifted one index to the left, and that means the index pointer is still pointing at the same index but there is now a different element at that index:

Variable	Value
new	[3, 2, 2, 0]
result	[6]
i	5
index	1: [3, → 2 ←, 2, 0]

The root cause of this problem is that we are mutating new at the same time that we are iterating over it. This is a big no-no. You should never mutate a data structure while you are processing it. Or, at the very least, you need to be very careful that the mutations you do perform do not cause you to incorrectly process or skip parts of it.

Next step is

result.push(i)

which means we push 5 to the end of result:

Variable	Value
new	[3, 2, 2, 0]
result	[6, 5]
i	5
index	1: [3, → 2 ←, 2, 0]

Alright, let's get to the next iteration: the pointer is again pushed forward to the next element, which leaves us with this picture:

Variable	Value
new	[3, 2, 2, 0]
result	[6, 5]
i
index	2: [3, 2, → 2 ←, 0]

First step:

i = new.max

Variable	Value
new	[3, 2, 2, 0]
result	[6, 5]
i	3
index	2: [3, 2, → 2 ←, 0]

Next step:

new.delete(i)

Variable	Value
new	[2, 2, 0]
result	[6, 5]
i	3
index	2: [2, 2, → 0 ←]

Next step:

result.push(i)

Variable	Value
new	[2, 2, 0]
result	[6, 5, 3]
i	3
index	2: [2, 2, → 0 ←]

As you can see, the iteration pointer is now at the end of the array, so there will be no next iteration. Therefore, the final value of the result array is [6, 5, 3], which we now join into a string and convert to an integer, so the final result is 653.

Fundamentally, there are four problems with your code:

1. Presumably, the if i = new.max combined assignment / conditional which always clobbers the value of i is a typo, and you meant to write if i == new.max.
2. You are mutating the new array while at the same time you are iterating over it.
3. You are deleting every occurrence of i from new, but you are only adding one occurrence to the result.
4. Your logic is wrong: if the current element is not the maximum element, you skip over it and ignore it completely. But if the current element is not the maximum element, that only means that it should appear in the output later, not that it should not appear at all.

If you only fix #1, i.e. you replace i = new.max with i == new.max and change nothing else about your code, the result will be 6. If you only fix #2, i.e. you replace for i in new with for i in new.dup (thus duplicating the new array and iterating over the copy so that mutating the new array itself does not influence the iteration), the result will be 65320. If you fix both #1 and #2, the result will be 65.

I encourage you to take pen and paper and trace all those three variants the same way we did above and fully understand what is going on.