I have seen it stated that it is undesirable, in a For...Next loop, e.g.,

For i = 1 to 10
'Do Something
Next

to change the index (i, in the above illustration) programatically.

1. Is this a commonly accepted taboo?

2. If so, why is it thought to be undesirable?

Thanks,
Alan Beban

The only danger that I can see is if the index gets set back, it could
create an infinite loop.

I have never experienced a problem just moving the index along its
step-path, even going beyond its loop limit.

It's like everything I guess, if you understand the implications, and know
the data, you can weigh up the risks.

--
---
HTH

Bob

(there's no email, no snail mail, but somewhere should be gmail in my addy)



"Alan Beban" <(E-Mail Removed)> wrote in message
news:%23cuf5Uj%(E-Mail Removed)...
>I have seen it stated that it is undesirable, in a For...Next loop, e.g.,
>
> For i = 1 to 10
> 'Do Something
> Next
>
> to change the index (i, in the above illustration) programatically.
>
> 1. Is this a commonly accepted taboo?
>
> 2. If so, why is it thought to be undesirable?
>
> Thanks,
> Alan Beban

Hi Alan.

I suppose that an iterative programming challenge might be OK for
programmatic index modification. But otherwise, the way I interpret this, is
that what is being changed must inherently be tightly coupled with what is
changing it. In the real world that could result in disparate code fragments
evaluating similar business logic differently, and might non-obviously have
to be maintained concurrently.

Tim

"Alan Beban" <(E-Mail Removed)> wrote in message
news:%23cuf5Uj%(E-Mail Removed)...
>I have seen it stated that it is undesirable, in a For...Next loop, e.g.,
>
> For i = 1 to 10
> 'Do Something
> Next
>
> to change the index (i, in the above illustration) programatically.
>
> 1. Is this a commonly accepted taboo?
>
> 2. If so, why is it thought to be undesirable?
>
> Thanks,
> Alan Beban

I personally avoid changing the index as I find it too difficult to debug
when things go wrong. The loops either run long (infinite loop possibly) or
end too soon and I end up patching things up in a bandaid kind of fashion.
Nothing written in stone here but I have never run into a situation that I
could not work around needing to change the index. The way I see it it is
kind of like Goto's... better to avoid them than to fix them.
--
HTH...

Jim Thomlinson


"Alan Beban" wrote:

> I have seen it stated that it is undesirable, in a For...Next loop, e.g.,
>
> For i = 1 to 10
> 'Do Something
> Next
>
> to change the index (i, in the above illustration) programatically.
>
> 1. Is this a commonly accepted taboo?
>
> 2. If so, why is it thought to be undesirable?
>
> Thanks,
> Alan Beban
>

Thanks to Bob Phillips, Tim Zych and Jim Thomlinson for taking the time
to respond (although I must say I wasn't able to follow Tim's response).
Just for the record, the situation that prompted my post was code to
delete all duplicate rows in a range. The following, in which RowsEqual
is a function that returns True or False depending on whether two rows
in a range or a two-dimensional array have all the same elements, was
what I coded:

Sub abtest4()
Dim rng As Range, i As Integer, j As Integer
Set rng = Range("DataRange").Rows
For i = rng.Rows.Count To 2 Step -1
For j = i - 1 To 1 Step -1
If RowsEqual(rng(i), rng(j)) Then
rng(i).Delete
i = i - 1
j = i
End If
Next j
Next i
End Sub

Alan Beban

Jim Thomlinson wrote:
> I personally avoid changing the index as I find it too difficult to debug
> when things go wrong. The loops either run long (infinite loop possibly) or
> end too soon and I end up patching things up in a bandaid kind of fashion.
> Nothing written in stone here but I have never run into a situation that I
> could not work around needing to change the index. The way I see it it is
> kind of like Goto's... better to avoid them than to fix them.

Your rng.Rows.Count is going to change as you delete rows and that is not a
good idea. Not onny are you changing i but you are also changing the range in
which it is moving...

This is just my preference but when I am deleteing I always create a range
object of all of the cells that want to delete within the loop structure and
then do one big delete of all of the cells I found at the end. It is more
effiecient and it avoids problems with the range changing while I am looking
through it. Just my two cents... With your code it would look like this...

Sub abtest4()
Dim rng As Range, i As Integer, j As Integer
dim rngToDelete as range

Set rng = Range("DataRange")
For i = rng.Rows.Count To 2 Step -1
For j = i - 1 To 1 Step -1
If RowsEqual(rng(i), rng(j)) Then
if rngToDelete is nothing then
Set rngtoDelete = rng(i)
else
Set rngToDelete = union(rngToDelete, rng(i))
end if
End If
Next j
Next i
if not rngToDelete is nothing then rngToDelete.Delete
End Sub
--
HTH...

Jim Thomlinson


"Alan Beban" wrote:

> Thanks to Bob Phillips, Tim Zych and Jim Thomlinson for taking the time
> to respond (although I must say I wasn't able to follow Tim's response).
> Just for the record, the situation that prompted my post was code to
> delete all duplicate rows in a range. The following, in which RowsEqual
> is a function that returns True or False depending on whether two rows
> in a range or a two-dimensional array have all the same elements, was
> what I coded:
>
> Sub abtest4()
> Dim rng As Range, i As Integer, j As Integer
> Set rng = Range("DataRange").Rows
> For i = rng.Rows.Count To 2 Step -1
> For j = i - 1 To 1 Step -1
> If RowsEqual(rng(i), rng(j)) Then
> rng(i).Delete
> i = i - 1
> j = i
> End If
> Next j
> Next i
> End Sub
>
> Alan Beban
>
> Jim Thomlinson wrote:
> > I personally avoid changing the index as I find it too difficult to debug
> > when things go wrong. The loops either run long (infinite loop possibly) or
> > end too soon and I end up patching things up in a bandaid kind of fashion.
> > Nothing written in stone here but I have never run into a situation that I
> > could not work around needing to change the index. The way I see it it is
> > kind of like Goto's... better to avoid them than to fix them.
>

Hi Jim,

My data range contained the following:

1 2 3 4
a b c d
1 2 3 4
a b c d
5 6 7 8
1 2 3 4
a b c d
1 2 3 4
a b c d
5 6 7 8

When I run my code on it the result is

1 2 3 4
a b c d
5 6 7 8

When I first attempted to run your code, I got a compiler error, Type
mismatch, at the indicated line. I then changed your code from

Set rng = Range("DataRange") to

Set rng = Range("DataRange").Rows with this result:

1 2 3 4
a b c d


5 6 7 8

Your comment did make me rethink my code and I simplified it to

Sub abtest4()
Dim rng As Range, i As Integer, j As Integer
Set rng = Range("DataRange").Rows
For i = rng.Rows.Count To 2 Step -1
For j = i - 1 To 1 Step -1
If RowsEqual(rng(i), rng(j)) Then
rng(i).Delete
End If
Next j
Next i
End Sub

Thanks for your interest. In case you wanted to check it yourself, here
is the RowsEqual function (watch for wordwrap):

Function RowsEqual(Row1, Row2)
'This function checks to see if two "rows"
'of an array or range are equal; it returns
'True if they are, False if they are not.

k = True

'Check to see that input rows are arrays or
'multicell ranges.
If (IsArray(Row1) And IsArray(Row2)) Then

'Convert input ranges to arrays.
arr1 = Row1: arr2 = Row2

'Loop to see if all elements are the same.
For i = LBound(arr1) To UBound(arr1)
For j = LBound(arr2) To UBound(arr2, 2)
If Not (arr1(i, j) = arr2(i, j)) Then RowsEqual =
False: Exit Function
Next
Next
RowsEqual = True

'If either input row is not an array or multicell
'range, give error message
Else
RowsEqual = "This function accepts only arrays and multicell
ranges"
End If

End Function


Jim Thomlinson wrote:
> Your rng.Rows.Count is going to change as you delete rows and that is not a
> good idea. Not onny are you changing i but you are also changing the range in
> which it is moving...
>
> This is just my preference but when I am deleteing I always create a range
> object of all of the cells that want to delete within the loop structure and
> then do one big delete of all of the cells I found at the end. It is more
> effiecient and it avoids problems with the range changing while I am looking
> through it. Just my two cents... With your code it would look like this...
>
> Sub abtest4()
> Dim rng As Range, i As Integer, j As Integer
> dim rngToDelete as range
>
> Set rng = Range("DataRange")
> For i = rng.Rows.Count To 2 Step -1
> For j = i - 1 To 1 Step -1
> If RowsEqual(rng(i), rng(j)) Then '<------Type mismatch
> if rngToDelete is nothing then
> Set rngtoDelete = rng(i)
> else
> Set rngToDelete = union(rngToDelete, rng(i))
> end if
> End If
> Next j
> Next i
> if not rngToDelete is nothing then rngToDelete.Delete
> End Sub

Alan,

Notwithstanding what I said earlier, I tend to agree with Jim in that I
would avoid setting an index from within an inner loop, the debugging could
get tricky! In other words, I don't like those risks :-).

Also, by saving up the deletes until the end as Jim does, you stop the
screen flashing as the screen gets re-painted when deleting.

I amended Jim's code and this worked for me

Sub abtest4()
Dim rng As Range, i As Integer, j As Integer
Dim rngToDelete As Range

Set rng = Range("DataRange").Rows
For i = rng.Rows.Count To 2 Step -1
For j = i - 1 To 1 Step -1
If RowsEqual(rng(i), rng(j)) Then
If rngToDelete Is Nothing Then
Set rngToDelete = Rows(i)
Else
Set rngToDelete = Union(rngToDelete, Rows(i))
End If
End If
Next j
Next i
If Not rngToDelete Is Nothing Then rngToDelete.Delete
End Sub

--
---
HTH

Bob

(there's no email, no snail mail, but somewhere should be gmail in my addy)



"Alan Beban" <(E-Mail Removed)> wrote in message
news:OLabCpm%(E-Mail Removed)...
> Hi Jim,
>
> My data range contained the following:
>
> 1 2 3 4
> a b c d
> 1 2 3 4
> a b c d
> 5 6 7 8
> 1 2 3 4
> a b c d
> 1 2 3 4
> a b c d
> 5 6 7 8
>
> When I run my code on it the result is
>
> 1 2 3 4
> a b c d
> 5 6 7 8
>
> When I first attempted to run your code, I got a compiler error, Type
> mismatch, at the indicated line. I then changed your code from
>
> Set rng = Range("DataRange") to
>
> Set rng = Range("DataRange").Rows with this result:
>
> 1 2 3 4
> a b c d
>
>
> 5 6 7 8
>
> Your comment did make me rethink my code and I simplified it to
>
> Sub abtest4()
> Dim rng As Range, i As Integer, j As Integer
> Set rng = Range("DataRange").Rows
> For i = rng.Rows.Count To 2 Step -1
> For j = i - 1 To 1 Step -1
> If RowsEqual(rng(i), rng(j)) Then
> rng(i).Delete
> End If
> Next j
> Next i
> End Sub
>
> Thanks for your interest. In case you wanted to check it yourself, here is
> the RowsEqual function (watch for wordwrap):
>
> Function RowsEqual(Row1, Row2)
> 'This function checks to see if two "rows"
> 'of an array or range are equal; it returns
> 'True if they are, False if they are not.
>
> k = True
>
> 'Check to see that input rows are arrays or
> 'multicell ranges.
> If (IsArray(Row1) And IsArray(Row2)) Then
>
> 'Convert input ranges to arrays.
> arr1 = Row1: arr2 = Row2
>
> 'Loop to see if all elements are the same.
> For i = LBound(arr1) To UBound(arr1)
> For j = LBound(arr2) To UBound(arr2, 2)
> If Not (arr1(i, j) = arr2(i, j)) Then RowsEqual = False:
> Exit Function
> Next
> Next
> RowsEqual = True
>
> 'If either input row is not an array or multicell
> 'range, give error message
> Else
> RowsEqual = "This function accepts only arrays and multicell
> ranges"
> End If
>
> End Function
>
>
> Jim Thomlinson wrote:
>> Your rng.Rows.Count is going to change as you delete rows and that is not
>> a good idea. Not onny are you changing i but you are also changing the
>> range in which it is moving...
>>
>> This is just my preference but when I am deleteing I always create a
>> range object of all of the cells that want to delete within the loop
>> structure and then do one big delete of all of the cells I found at the
>> end. It is more effiecient and it avoids problems with the range changing
>> while I am looking through it. Just my two cents... With your code it
>> would look like this...
>>
>> Sub abtest4()
>> Dim rng As Range, i As Integer, j As Integer
>> dim rngToDelete as range
>>
>> Set rng = Range("DataRange")
>> For i = rng.Rows.Count To 2 Step -1
>> For j = i - 1 To 1 Step -1
>> If RowsEqual(rng(i), rng(j)) Then '<------Type mismatch
>> if rngToDelete is nothing then
>> Set rngtoDelete = rng(i)
>> else
>> Set rngToDelete = union(rngToDelete, rng(i))
>> end if
>> End If
>> Next j
>> Next i
>> if not rngToDelete is nothing then rngToDelete.Delete
>> End Sub

Thanks for your continued interest.

The screen flashing can be eliminated by turning off ScreenUpdating
during the running of the code. Can you tell me a little bit more
specifically about possible tricky debugging?

Thanks,
Alan Beban

Bob Phillips wrote:
> Alan,
>
> Notwithstanding what I said earlier, I tend to agree with Jim in that I
> would avoid setting an index from within an inner loop, the debugging could
> get tricky! In other words, I don't like those risks :-).
>
> Also, by saving up the deletes until the end as Jim does, you stop the
> screen flashing as the screen gets re-painted when deleting. . . .

Alan,

By that I was really referring to the principle. Whilst I have no aesthetic
problems with manually incrementing the index (probably at odds with most in
that, but hey!), I did mention that I do when so, it is when I am pretty
clear what I am letting myself in for. If I am setting the index of a loop
from within another loop, I would worry that if it didn't work first time,
the debugging would be complex because even though I know what the value is
within the inner loop, it immediately changes again on exit. And there is
the inner loop index to watch as well, it all adds to the complexity, which
is why I would avoid it if I could.

As to screenupdating, you are of course absolutely right, but saving the
deletes up is also quicker. Or at least it is until the dataset gets large,
there is a point where it gets more and more inefficient. In these cases,
autofilter works better. And of course, identifying the target rows with a
formula is relatively trivial (especially compared to a double loop with the
outer being adjusted in the inner :-)), and I believe you are a formula man
so it should be a breeze for you.

I am not sure what you are trying to get out of this discussion, whether you
just want some input on the manually adjusting indexes, or techniques
generally, but I hope this is helpful. If you are not interested in my
asides, just let me know.

--
---
HTH

Bob

(there's no email, no snail mail, but somewhere should be gmail in my addy)



"Alan Beban" <(E-Mail Removed)> wrote in message
news:%23gP68Sv%(E-Mail Removed)...
> Thanks for your continued interest.
>
> The screen flashing can be eliminated by turning off ScreenUpdating during
> the running of the code. Can you tell me a little bit more specifically
> about possible tricky debugging?
>
> Thanks,
> Alan Beban
>
> Bob Phillips wrote:
>> Alan,
>>
>> Notwithstanding what I said earlier, I tend to agree with Jim in that I
>> would avoid setting an index from within an inner loop, the debugging
>> could get tricky! In other words, I don't like those risks :-).
>>
>> Also, by saving up the deletes until the end as Jim does, you stop the
>> screen flashing as the screen gets re-painted when deleting. . . .