Upgrading to .NET, help ?

[Netherdrake]

I have upgraded 16591 lines of code program from VB6 to .NET.
There was a lot of errors and warning that i fixed but i stuck on those.

First one comes here:

	Function FoundBotName(ByRef thebotname As String) As Object
		Dim check As Short
		'UPGRADE_WARNING: Couldn't resolve default property of object FoundBotName.
		FoundBotName = True
		Do 
			check = check + 1
			If LCase(BotName(check)) = LCase(thebotname) Then Exit Function
		Loop Until check = BotMax
		'UPGRADE_WARNING: Couldn't resolve default property of object FoundBotName.
		FoundBotName = False
	End Function

Couldn't resolve default property of object FoundBotName

Second one:

    Function BotsInSector(ByRef theSectorNumber As Short, ByRef theZone As Short) As String
        Dim check As Short
        Do
            check = check + 1
            If BotSector(check) = theSectorNumber And theZone = BotZone(check) And BotHull(check) <> 0 Then
                BotsInSector = "," & BotShipClass(check) & BotShipKind(check) & BotName(check) & "," & BotGuild(check) & BotsInSector
            End If
        Loop Until check = BotMax
        Return Nothing
    End Function

Variable BotsInSector is used before it has been assigned a value. A null reference exception could result at runtime.

And the last one:

	Function HomeEncrypt(ByRef Message As String, ByRef ****youcodetheif As Short) As String
        If Len(Message) = 0 Then Exit Function

Function HomeEncrypt doesn't return a value on all code paths. A null reference exception could occurs at runtime when the result is used.

Thanks in advance.

- Netherdrake

 

[jmcilhinney]

While it's strictly legal, you shouldn't use the function name to return a value in VB.NET. This function:

	Function FoundBotName(ByRef thebotname As String) As Object
		Dim check As Short
		'UPGRADE_WARNING: Couldn't resolve default property of object FoundBotName.
		FoundBotName = True
		Do 
			check = check + 1
			If LCase(BotName(check)) = LCase(thebotname) Then Exit Function
		Loop Until check = BotMax
		'UPGRADE_WARNING: Couldn't resolve default property of object FoundBotName.
		FoundBotName = False
	End Function

should be rewritten like this:

Function FoundBotName(ByVal thebotname As String) As Object
    Dim result As Boolean = False

    For Each name As String In BotName
        If String.Equals(name, thebotname, StringComparison.CurrentCultureIgnoreCase) Then
            result = True
            Exit For
        End If
    Next

    Return result
End Function

That should give you an idea about the others as well. In VB.NET, you should NEVER use Exit Function. You should always return a value on the last line of the method.

 

[Netherdrake]

Actually, i not code VB.NET (i can't). I code VB6 about 3 years, and i am not so good in .NET as i am in VB6, so can you tell me where can i find a way to learn VB.NET.
Some e-books, tutorials, etc.

 

[JohnH]

You should always return a value on the last line of the method.

I don't agree, sometimes it is best to Return when it is logical to do that. Caching return value, perhaps forcing later conditional handling of that, and later again actually return it may add unnecessary complexity and abstraction to the code. You may argue that it is easier to relate to any function that always has the Return statement at last line, that this adds predictability and conformity - but with that abstraction it may be a lot harder to backtrack where in the previous code structures that value came from. Making explicit Return statements exactly when the return value is determined adds much to readability in my opinion.

I would rewrite it like this:

Function FoundBotName(ByVal thebotname As String) [U]As [B]Boolean[/B][/U]

    For Each name As String In BotName
        If String.Equals(name, thebotname, StringComparison.CurrentCultureIgnoreCase) Then
            [U]Return True[/U]
        End If
    Next

    Return False ' can be omitted, default return value of Boolean is False
End Function

 

[jmcilhinney]

Caching return value, perhaps forcing later conditional handling of that, and later again actually return it may add unnecessary complexity and abstraction to the code. You may argue that it is easier to relate to any function that always has the Return statement at last line, that this adds predictability and conformity - but with that abstraction it may be a lot harder to backtrack where in the previous code structures that value came from.

There are certainly different schools of thought and neither is definitively wrong or right, but I would suggest that if following my advice genuinely does make the code harder to follow then, in the vast majority of cases, your methods are poorly designed and need refactoring. I've never seen a case where methods that follow the single responsibility principle are made more complex by ending with a single Return statement.

 

[picoflop]

Are we going to vote?
One for JohnH.

Leave with some meaningfull return value if it makes sense, I'd say.

Private Function FoundSomePattern(ByVal s As String, ByVal p As String) As Boolean

        If s.Length = 0 OrElse p.Length = 0 Then Return False
        ' 100 lines of code to find a pattern in a string
        Return FoundSomePattern

    End Function

If you read the code you see, that the function is done if s or p is empty. Easy to read. Otherwise you'd have to read lines of code to check if maybe later FoundSomePattern is set to either true or false. Maybe because there is another condition that might result in a true instead of a false?

BTW: One vote against JohnH , because ommiting a "default" return makes the code harder to read for ppl who can program, but have no experience with (e.g.) vb.net.

 

[JohnH]

If you read the code you see, that the function is done if s or p is empty. Easy to read.

It is correct that when both parameters here are valid and Length is 0 you can return a False result right away without further checking the contents (which is empty anyway). But what if either of those parameters are not valid? If either parameter here is a null reference (Nothing) you should throw an InvalidArgumentException naming the offending parameter, and not implicitly have an unhandled NullReferenceException bubble up. Would you cache this result also, and wait to end of method to throw the Exception? No, you would throw it right there when it was detected.

ommiting a "default" return makes the code harder to read

That's why I still left that default return explicitly (but perhaps I shouldn't have indicated that omitting it is a valid approach?) On the argument "ppl who can program, but have no experience with vb.net" I wouldn't worry too much, ppl developing with VB.Net have an obligation of learning VB.Net (to the relevant extent) or else they will most likely write bad VB.Net code or make bad development decisions. Even if you do know the default value of all value types it is less strainful to read when you don't have to implicitly make that mental lookup each time you read it. Or is it? This borders to the "If Checked = True Then" discussion, where I favour the "If Checked Then" approach.

On the previous matter, in case this was misinterpreted, using the function name to return a value is very different from using the Return statement. When you use the function name the result is cached (using the function name as variable) and is not returned until the last line in method is executed (same as JMs approach), while the Return statement exits the method and return immediately.

 

[Netherdrake]

OK guys, it looks like i fix almost everything, only two problems stay's unfixed.
It's Function 'function name' doesn't return value on all code paths. bla bla bla can cause runtime, whatever" and Could not resolve default property of object 'object name'

I was follow the instructions that u guys give me about this in the "FoundBotName" Sub, but they didn't help, i need explanation like on a beginner.

However here's the code:

	Sub PurchaseContras(ByRef check As Short, ByRef theGoodType As Short, ByRef theGoodAmmount As Short)
        WriteSub("purchasegoods")
        Dim check2 As Short
        Dim difference As Short
        Dim differenece As Short
        Dim temp As Short
        Dim temp2 As Short
        Dim redo As Boolean
        check2 = UserCheckToGuild(check)
        difference = 25
        temp2 = theGoodAmmount
RedoSpot:
        theGoodAmmount = temp2
        redo = False
        If theGoodAmmount > 200 Then
            redo = True
            temp2 = theGoodAmmount - 200
            theGoodAmmount = 200
        End If
        If theGoodAmmount < -200 Then
            redo = True
            temp2 = theGoodAmmount + 200
            theGoodAmmount = -200
        End If
        If theGoodAmmount = 0 Then Exit Sub
        If UserCargoLeft(check) < theGoodAmmount Then Exit Sub
        If UserCargoLeft(check) - theGoodAmmount < 0 Then Exit Sub
        If UserLanded(check) = False Then Exit Sub
        'UPGRADE_WARNING: Couldn't resolve default property of object UserContra(). Click for more: 'ms-help://MS.VSCC.v90/dv_commoner/local/redirect.htm?keyword="6A50421D-15FE-4896-8A1B-2EC21E9037B2"'
        temp = UserContra(check, theGoodType)
        If temp + theGoodAmmount < 0 Then Exit Sub
        If theGoodAmmount > 0 Then
            Select Case theGoodType

Couldn't resolve default property of object UserContra() - wtf ?

And this one:

    Function BotsInSector(ByRef theSectorNumber As Short, ByRef theZone As Short) As String
        Dim check As Short
        Do
            check = check + 1
            If BotSector(check) = theSectorNumber And theZone = BotZone(check) And BotHull(check) <> 0 Then
                BotsInSector = "," & BotShipClass(check) & BotShipKind(check) & BotName(check) & "," & BotGuild(check) & BotsInSector
            End If
        Loop Until check = BotMax
    End Function

Function 'BotsInSector' doesn't return value on all code paths. A null reference may result in a runtime.
Also, Variable BotsInSector is used before it has been assigned a value. A null reference may result in a runtime. in the same function.

Thanks really a lot =D

 

[JuggaloBrotha]

For the not returning a value on all code paths:

Function BotsInSector(ByRef theSectorNumber As Short, ByRef theZone As Short) As String
    Dim Output As String = String.Empty
    Dim check As Short
    Do
        check += 1
        If BotSector(check) = theSectorNumber And theZone = BotZone(check) And BotHull(check) <> 0 Then
            Output = "," & BotShipClass(check) & BotShipKind(check) & BotName(check) & "," & BotGuild(check) & BotsInSector
        End If
    Loop Until check = BotMax
    Return Output
End Function

 

[Netherdrake]

it still says the message:
Variable 'BotsInSector' it used before it has been assigned a value. Null reference may cause runtime.

 

[jmcilhinney]

You should be using the 'Output' variable wherever you were previously using the function's implicit local variable, i.e. BotsInSector. I think JB just missed one:

Output = "," & BotShipClass(check) & BotShipKind(check) & BotName(check) & "," & BotGuild(check) & [COLOR="Red"]BotsInSector[/COLOR]

should be:

Output = "," & BotShipClass(check) & BotShipKind(check) & BotName(check) & "," & BotGuild(check) & [COLOR="Green"]Output[/COLOR]

 

[Netherdrake]

So could any1 help me with the last one:

	Sub PurchaseContras(ByRef check As Short, ByRef theGoodType As Short, ByRef theGoodAmmount As Short)
        WriteSub("purchasegoods")
        Dim check2 As Short
        Dim difference As Short
        Dim differenece As Short
        Dim temp As Short
        Dim temp2 As Short
        Dim redo As Boolean
        check2 = UserCheckToGuild(check)
        difference = 25
        temp2 = theGoodAmmount
RedoSpot:
        theGoodAmmount = temp2
        redo = False
        If theGoodAmmount > 200 Then
            redo = True
            temp2 = theGoodAmmount - 200
            theGoodAmmount = 200
        End If
        If theGoodAmmount < -200 Then
            redo = True
            temp2 = theGoodAmmount + 200
            theGoodAmmount = -200
        End If
        If theGoodAmmount = 0 Then Exit Sub
        If UserCargoLeft(check) < theGoodAmmount Then Exit Sub
        If UserCargoLeft(check) - theGoodAmmount < 0 Then Exit Sub
        If UserLanded(check) = False Then Exit Sub
        'UPGRADE_WARNING: Couldn't resolve default property of object UserContra().
        temp = UserContra(check, theGoodType)
        If temp + theGoodAmmount < 0 Then Exit Sub
        If theGoodAmmount > 0 Then
            Select Case theGoodType

UPGRADE_WARNING: Couldn't resolve default property of object UserContra().

 

[Netherdrake]

~ bumping ~ bumping ~ bumping ~ bumping ~ bumping

 

[JohnH]

Taking a wild guess, UserContra is a function that you haven't defined return type for?

 

[Netherdrake]

I know that but what to return ?
Just:

Dim result As Boolean = False 'at the beginning

And before the End Function:
Return result ' right ?