1

I work at $COMPANY and I'm helping maintain $LEGACY_APPLICATION. It's written in visual basic 6.

I was faced with doing an unpleasantly elaborate nested if statement due to the lack of VB6's ability to perform short circuit evaluations in if statements (which would simplify this a lot). I've tried AndAlso, but to no avail. Must be a feature added after VB6.

Some genius on SO somewhere pointed out that you can trick a select case statement into working like a short-circuiting if statement if you have the patience, so I tried that, and here's what I came up with:

Select Case (True) ' pretend this is an if-else statement
    Case (item Is Nothing): Exit Sub ' we got a non-element
    Case ((item Is Not Nothing) And (lastSelected Is Nothing)): Set lastSelected = item ' we got our first good element
    Case (item = lastSelected): Exit Sub ' we already had what we got
    Case (Not item = lastSelected): Set lastSelected = item ' we got something new
End Select

It's definitely a little unusual, and I had to make use of my fantastic whiteboard (which, by the way, is pretty much the most useful programming resource besides a computer) to make sure I had mapped all of the statements correctly.

Here's what's going on there: I have an expensive operation which I would like to avoid repeating if possible. lastSelected is a persistent reference to the value most recently passed to this calculation. item is the parameter that was just received from the GUI. If there has never been a call to the program before, lastSelected starts out as Nothing. item can be Nothing too. Additionally, if both lastSelected and item are the same something, skip the calculation.

If I were writing this in C++, I would write:

if (item == NULL || (lastSelected != NULL && item->operator==(*lastSelected))) return;
else lastSelected = item;

However, I'm not.

Question

How can I rewrite this to look better and make more sense? Upvotes will be awarded to answers that say either "YES and here's why: X, Y, Z" or "NO, and here's why not: X, Y, Z".

Edits

Fixed the C++ statement to match the VB6 one (they were supposed to be equivalent)

Wug
  • 12,956
  • 4
  • 34
  • 54
  • It would help to mention that I've only been using visual basic for about a month but I have a solid background in other, modern languages. – Wug Jul 20 '12 at 05:56
  • VB6 and C++ snippet are not equivalent. Do you want C++ snippet translated in efficient VB6 code? – wqw Jul 20 '12 at 11:37
  • That was the hope. I'll settle for pretty too code too if you can manage it, but efficiency is a priority. – Wug Jul 20 '12 at 13:20

4 Answers4

4

This is shorter and 100x more readable.

EDIT Wug edited the code in MarkJ's original answer, into this:

If (item Is Nothing)
    Then Exit Sub ' we got a non-element
ElseIf (lastSelected Is Nothing) Then
    Set lastSelected = item ' we got our first go 
ElseIf (item = lastSelected) Then
    Exit Sub ' we already had what we got
End If
Set lastSelected = item ' we got something new

Here's MarkJ's edit in response. One nested if, but only one Set. Seems neater to me. 

If (item Is Nothing) Then 
  Exit Sub ' we got a non-element 
ElseIf Not (lastSelected Is Nothing) Then ' not our first go
  If (item = lastSelected) Then 
    Exit Sub ' we already had what we got 
  End If 
End If
Set lastSelected = item ' we got something new
' does stuff here? @Wug is that true?
  • To compare reference equality in VB6 use item Is LastSelected. Because item = lastSelected will probably evaluate the default properties in the objects and compare those instead!
  • Since brevity appears to be a goal, consider this. If you Exit Sub when condition X is True, you don't need to check X again later. It is False! Unless it changes its value in between evaluations (e.g. X is a function that checks the system clock). You were checking whether item was lastSelected, then whether it wasn't. And if item Is Nothing is False, do not bother to check whether item Is Not Nothing is True!
  • VB6 does not short circuit for backwards compatibility with ancient versions of Basic
  • Stop worrying that VB6 is not some other language and relax!
Community
  • 1
  • 1
MarkJ
  • 30,070
  • 5
  • 68
  • 111
  • I don't want to reference compare because there is the possibility that I won't have the same reference. However, the reference comes from something the user clicks in the GUI and it's possible that if they click something stupid it will end up with no reference. – Wug Jul 20 '12 at 05:53
  • Or at least it might be. It's a listview. What a contrived, obnoxious piece of derp. 1 indexed arrays. What were they thinking. – Wug Jul 20 '12 at 05:54
  • I really don't like it. Languagologically, it's ugly, and really easy to shoot yourself in the foot. Not in little ways like C either, where you cause yourself segment faults. I mean like subtle, sinister, O(n^3) or worse ways that stick to your code like little fragments of shrapnel blown from the grenades of hell itself. – Wug Jul 20 '12 at 05:59
  • That said, part of the reason the codebase I'm working with is so funky is because it's been partially written by people who didn't know what they were doing, so my arcane use of a switch statement, though clearly documented, probably wouldn't do the codebase any good. – Wug Jul 20 '12 at 06:09
  • @Wug. 1-indexed arrays? They were thinking of maintaining backwards compatibility with existing BASIC code written over the previous 30 years. And a good thing too. If only Microsoft had maintained backwards compatibility in VB.Net, you'd be working on that project in Visual Studio 2010, not VB6. – MarkJ Jul 20 '12 at 12:19
  • @Wug. Seriously, and with all respect, relax! VB is a different language and it's not C-based. You have a little learning to do if you're going to program VB, and you'll be a little less productive while you learn. It's not your fault, and it's not VB's fault. If you want to limit yourself to only C-like languages, close the VB6 IDE and do something else! If you're going to learn VB, just relax and enjoy it! – MarkJ Jul 20 '12 at 12:31
  • 1
    *turns into a ball of tension and explodes, shards of wug fly everywhere.* Anywhoo, this got an upvote. – Wug Jul 20 '12 at 14:06
  • @wug (if you are there, rather than still lying in shards!) I responded to your edit of my logic with another suggestion, which I think is better. Thanks for fixing parentheses typo though. Please can you leave a comment if you edit the logic again? Otherwise the system doesn't notify me & I won't necessarily notice. I feel a sense of ownership over the answer & would like to see edits – MarkJ Jul 21 '12 at 06:56
  • 1
    I don't have full edit privileges. You forgot a ( on line 4: `If item = lastSelected) Then` – Wug Jul 21 '12 at 07:19
3

YES

I translated it from your case statement. I find it easier to read, personally.

If Item Is Nothing Then
  Exit Sub ' we got a non-element
ElseIf LastSelected Is Nothing Then
  Set LastSelected = Item ' we got our first good element
ElseIf Item = LastSelectedItem Then
  Exit Sub ' we already had what we got
Else
  Set LastSelected = Item ' we got something new
End If

You asked for explanation. I tried not to have to give much (by re-using your own code comments).

But here it is anyway :-)

  • Firstly if there is no item, just exit. Easy.
  • Otherwise if LastSelected Is Nothing then we know, because the first if condition failed, that Item exists, and it's safe to mark that value as having been Last Selected. As you say, we got our first good element. The sub continues on.
  • However if we have existing values for Item and LastSelected, then either they are equal or not. If they are equal, just quit.
  • If they aren't equal, then update LastSelected. As you say, we got something new.
azhrei
  • 2,303
  • 16
  • 18
  • I'm more looking for an explanation of why this way is better than the other way. (And why VB doesn't include short circuiting boolean evaluation) – Wug Jul 20 '12 at 05:42
  • 1
    I think it's better because it is done in the lingo of the language :-). The case statement is a workaround. You can read further here for why VB doesn't include short circuiting: http://stackoverflow.com/questions/1445867/why-would-a-language-not-use-short-circuit-evaluation – azhrei Jul 20 '12 at 05:52
  • 1
    @Wug, indexes start at 1, there is no short-circuiting, weird print statements et al. exist because that's the way it was done in GW/QBasic, even today VB.net requires special *Also usage to short circuit. An `If` block is usually more readable but `case true`'s short-circuit semantics can be handy to test then call accross a reference; http://stackoverflow.com/questions/3242560/andalso-orelse-in-vba – Alex K. Jul 20 '12 at 09:54
  • +1 because this is good, although mine's shorter & IMHO better. Hmm, now this has 3 upvotes, and mine only 2. Sob! – MarkJ Jul 21 '12 at 07:02
1

You can use a helper function like this:

Private Function pvGetItemData(oItem As ListItem) As Variant
    If Not oItem Is Nothing Then
        pvGetItemData = oItem.Tag
    Else
        pvGetItemData = -1
    End If
End Function

and then

If pvGetItemData(Item) = pvGetItemData(LastSelected) Then
    ' cache hit
Else
    ' do calc
    Set LastSelected = Item
End If
wqw
  • 11,771
  • 1
  • 33
  • 41
  • I thought about doing it this way. why, in your opinion, is it better? Edit plawcks. – Wug Jul 20 '12 at 13:16
  • 1
    It's more readable. VB6's lack of short-circuit `If` makes code more verbose but it can still be readable. Short-circuit evaluation is not readable per se. Follow simple guidelines "never put two statements on one line with :" or "always start `If`/`Else` body on new line" and you can make this code snippet palatable. – wqw Jul 20 '12 at 13:35
  • This is very nice but it has different behaviour to original snippets. If item is Nothing, and lastSelected is Nothing, this reports cache hit whereas original code would Exit Sub. I suppose we don't know what happens after the code snippet, and whether it matters if the code continues – MarkJ Jul 21 '12 at 06:38
0

YES

I'd make that simpler:

If item Is Nothing Then
  Exit Sub ' we got a non-element
Else
  Set lastSelected = item ' we got something to assign
End If

Unless there are side effects assigning lastItem (it can be property with invalid assignment code), then code logic is essentially same.

If you are not required to exit the sub (snippet is at the end of sub or something), then next is even simpler:

If Not (item Is Nothing) Then Set lastSelected = item

BTW, your Select Case (True) looks really odd to VB programmer :)

Arvo
  • 10,349
  • 1
  • 31
  • 34
  • Thing about this is, the whole reason I track the previous value is so I can cache the result of the operation, and this doesn't do that. I must check both values for nothing before comparing, otherwise I end up with errors – Wug Jul 20 '12 at 06:32
  • 1
    Well, then MarkJ answer (code) is most natural in VB6. You might not like it, but your switch-like construct is IMO much worse. Think about poor VB coder, who will fix this code next year :) – Arvo Jul 20 '12 at 08:16
  • One more thing - you should not save and compare references, but actual values (text or itemdata or something derived from these), what your extensive calculation uses. This way you have no problems with null values (Nothing) or two different pointers (object references) pointing to same object. – Arvo Jul 20 '12 at 08:31
  • @Arvo Thanks for compliment. Feel free to upvote my answer if you like it :) – MarkJ Jul 20 '12 at 12:21
  • *BTW, your Select Case (True) looks really odd to VB programmer :)* It looks really odd to me as a non VB programmer too. I didn't upvote this because I can't use it: It doesn't have the same outcomes as the presented case. – Wug Jul 20 '12 at 14:12