Coding Suggestions...

Topics: Developer Forum
Nov 2, 2006 at 2:07 PM
OK, so I'm a bit of a clean code fanatic, and I like to use the features of the language. With that in mind there are some suggestions for the code side of things:

a) use Generics for your list arrays. the itemArray could be a List(Of Item) rather than a plain ArrayList, the array of monsters could be a List(Of Monster) (more on Monster later).

b) Avoid use of Object wherever possible, an example might be Hero.RightHand is currently object - this could be changed to Item, or Weapon if only weapons are allowed in the right hand. Not only does this provide strong type help but intellisense also works better.

c) Split classes out into their own files and/or use namespaces or even just folders. There is currently a clash between the class Monster and the module Monster - the Monster class isn't in the Monster.vb file for example. By default VB does not create seperate namespaces for sub folders so that might be an easy way of managing the code (an Items folder, Avatar folder etc...

d) Refactor some code into seperate functions - a specific example I would do this with would be the ShowBackpack method. Apart from the fact that it seems to sometimes return a string and sometimes return an item it does a lot of stuff.

e) Avoid ByRef parameters where possible. The ShowAllItems method is an example which doesn't need a ByRef ItemArray...

e.g. Needs ByRef

Dim array as ArrayList
Dim success as Boolean = FillArray(array);

Public Function RunMethod(ByRef refArray As ArrayList) As Boolean

If refArray Is Nothing Then refArray = New ArrayList()
' Do something that return true or false for the function and updates return array.
' The newly created array is returned

End Function

e.g. Doesn't need ByRef

Dim array as New ArrayList()
Dim success as Boolean = FillArray(array);

Public Function FillArray(ByVal valArray As ArrayList) As Boolean

' Cannot set valArray to new array as this would not be passed back. So throw exception if its Nothing
If valArray Is Nothing Then Throw New ArgumentNullException("valArray")
valArray.Clear()
' Do something that return true or false for the function and updates return array.

End Function

Basically, you don't need ByRef if you are just manipulating the properties or methods of an existing reference object. An exception to this is a string, which is not mutable so needs to be passed by reference if you want the new string to be returned. Obviously value objects (integers, dates etc) need to passed byref if the new value is to be returned. In the above ShowAllItems method then startLetter, itemCount and yPos do need to be passed by reference.

f) Also consider running some of the Code Analysis tools (if you have Visual Studio Pro) on the code.

g) What about unit tests? Too much perhaps? :)
Nov 2, 2006 at 2:12 PM
Oh...

h) Consider turning Option Strict On.

Line 393 of DunGen3.vb gives this:

Digger(startpos.x, startpos.y - intCtr, Z, SquareType.Floor)

but the fourth parameter should be a Boolean (SquareType.Floor = 3). Don't know what this code should be doing but its probably doing it wrong.
Nov 2, 2006 at 4:08 PM
The last bit here could be the reason for the Room Generation error (perhaps).


Plus, I'd obviously like to help by changing any of these things if you'd like...
Coordinator
Dec 20, 2006 at 2:20 AM
parameters 4 & 5 are both optional, with different datatypes. the sub is smart enough to know which param is being passed since one is integer and one is boolean.

Coordinator
Dec 20, 2006 at 2:26 AM
a) use Generics for your list arrays. the itemArray could be a List(Of Item) rather than a plain ArrayList, the array of monsters could be a List(Of Monster) (more on Monster later).

considering this.

***************

b) Avoid use of Object wherever possible, an example might be Hero.RightHand is currently object - this could be changed to Item, or Weapon if only weapons are allowed in the right hand. Not only does this provide strong type help but intellisense also works better.

this is done because I dont want to limit what the hero can hold to just Weapon or Potion, etc... I'll see if I can use Item.

***************

c) Split classes out into their own files and/or use namespaces or even just folders. There is currently a clash between the class Monster and the module Monster - the Monster class isn't in the Monster.vb file for example. By default VB does not create seperate namespaces for sub folders so that might be an easy way of managing the code (an Items folder, Avatar folder etc...

I am gradually moving some of the classes into files.

***************

d) Refactor some code into seperate functions - a specific example I would do this with would be the ShowBackpack method. Apart from the fact that it seems to sometimes return a string and sometimes return an item it does a lot of stuff.

It does a lot. I'll see where I can refactor.

***************

e) Avoid ByRef parameters where possible. The ShowAllItems method is an example which doesn't need a ByRef ItemArray...

As far as I know, I use them sparingly. I'll take a look at the one you mention.

***************
Dec 21, 2006 at 9:53 AM
>> parameters 4 & 5 are both optional, with different datatypes.
>> the sub is smart enough to know which param is being passed
>> since one is integer and one is boolean.

Thats not quite right.

Example:

Public Sub Method1(p1 As String, Optional p3 As Integer = -1, Optional p2 As Boolean = True)

The following is the correct way to specify parameters 1 and 3:

Method1( "Hello", , False)

The following is NOT the correct way:

Method1( "Hello", False)

What this will do is do an automatic conversion of the False value to an Integer, so p2 will be set to 0 (or whatever False is as a number), and p3 will be True (its default).