I am creating an add-in for a piece of software our company uses, so I'm working pretty extensively with that program's API. There are many COM object members which return or contain an array that I need to work with. Sometimes functions will return an uninitialized array (Null or "Nothing" return) or sometimes they may return an initialized array that is not allocated. My goal is to eliminate redundant code in the form of If
/Else
blocks before using each array, or Try
/Catch
blocks to handle exceptions thrown by the array being un-initialized or un-allocated.
Note: I'm not trying to validate any data here - that can be done in the calling code or in a function or whatever - I'm simply looking to determine if an array is in a usable state (e.g. initialized and allocated with at least one member)
I've created a member extension for arrays to check if the array is both initialized and allocated.
I'd like to get some input on whether or not this is a well designed solution or if it's total bupkis, if I'm missing anything else I should be checking for, and if improvements can be made in terms of robustness, efficiency, or general best practices; I would like to make it as fast as possible for dealing with large arrays.
Imports System.Runtime.CompilerServices
Module Module1
#Region "Extension Methods"
<Extension()>
Public Function IsAllocated(ByVal inArray As System.Array) As Boolean
Dim FlagEx As Boolean = True
Try
If inArray Is Nothing Then
FlagEx = False
ElseIf inArray.Length <= 0 Then
FlagEx = False
ElseIf inArray(0) Is Nothing Then
FlagEx = False
End If
Catch ex As Exception
FlagEx = False
End Try
Return FlagEx
End Function
#End Region
End Module
2 Answers 2
Option Strict
First thing first: Enable
Option Strict
please read : whats-an-option-strict-and-explicitThis will make your code less error prone. If you have enabled
Option Strict
you will see a warning on the lineElseIf inArray(0) Is Nothing Then
that late binding is not allowed if usingOption Strict On
.
Regions
Please read are-regions-an-antipattern-or-code-smell
Is there a good use for regions?
No. There was a legacy use: generated code. Still, code generation tools just have to use partial classes instead. If C# has regions support, it's mostly because this legacy use, and because now that too many people used regions in their code, it would be impossible to remove them without breaking existent codebases.
Think about it as about goto. The fact that the language or the IDE supports a feature doesn't mean that it should be used daily. StyleCop SA1124 rule is clear: you should not use regions. Never.
Hungarian notation
The parameter name
inArray
is a sign, that you are misusing the original intention. Please read this: what-is-the-benefit-of-not-using-hungarian-notationAnd don't forget to read the reffered links.
That beeing said let us check how we could enhance this method by grouping the conditions together.
By using OrElse
or AndAlso
we can short circuit conditions, which means that if a conditionA is true
the conditionB won't be evaluated. like
If conditionA OrElse conditionB Then
End If
If we invert the conditions we can simplify the whole method to
Module Module2
<Extension()>
Public Function IsAllocated(ByVal inArray As System.Array) As Boolean
Return Not ((inArray Is Nothing) OrElse (inArray.Length = 0) OrElse (inArray.GetValue(0) Is Nothing))
End Function
End Module
-
\$\begingroup\$ Hmm... Idk about the Hungarian here. Seems more like a rare case of doing it right. And, as always, we disagree about regions in general. However, they're not needed here. ++ \$\endgroup\$RubberDuck– RubberDuck2015年06月30日 15:03:47 +00:00Commented Jun 30, 2015 at 15:03
-
\$\begingroup\$ I use "in" and "out" prefixes for arguments identify arguments as inputs (provided with some value and evaluated by the function) vs outputs (provided as empty or null objects and populated by the function for use by the calling procedure). I've advocated Hungarian notation in VB6/VBA, but in Visual Studio .Net it really seems unnecessary 99% of the time as the IDE does a great job of showing variable types. BUT, there is no way (that I know of) in VB.Net to specify an argument as an input or output, as such the IDE cannot show you this info so I use hungarian to tell me. \$\endgroup\$CBRF23– CBRF232015年06月30日 15:22:17 +00:00Commented Jun 30, 2015 at 15:22
-
2\$\begingroup\$ It is not about the
in
but about theArray
. If you change the type in the future but keep the parameter name the name is lying. A better name would be to call it justinput
then. \$\endgroup\$Heslacher– Heslacher2015年06月30日 15:28:19 +00:00Commented Jun 30, 2015 at 15:28 -
1\$\begingroup\$ Gotcha - but seeing as this is an extension for the type System.Array, it's not very likely that I would change the type in the future ;) However, your note makes sense and is well received! --- Also, with regards to Regions, this is totally new to me and seemed like a good way of keeping organized. I can see how this could be considered an "anti-pattern" though after reviewing the link you provided. I guess coming from VB6 regions just seem like a great new tool since you had a minimum of 6 lines for a basic Get/Set property wrapper! \$\endgroup\$CBRF23– CBRF232015年06月30日 15:40:54 +00:00Commented Jun 30, 2015 at 15:40
-
\$\begingroup\$ About the regions I felt the same as I switched from VB6 to NET. Now I don't like nor use them anymore. \$\endgroup\$Heslacher– Heslacher2015年06月30日 15:43:26 +00:00Commented Jun 30, 2015 at 15:43
It's not bad. An extension method is a nice choice here. There's no state and you're extending a class that you don't own/can't directly modify. Good design decision. I also like that you're using the Try...Catch
paradigm. A common mistake for VB6 devs moving to .Net is carrying over the VB6 error handling pattern.
One thing I don't like about what you've done is this.
Catch ex As Exception
This catches every possible exception in the world, including potentially fatal memory errors. Be as specific as possible when catching exceptions. What specific exception would you expect to happen? Catch that instead, or none at all.
The only other thing I want to mention is that FlagEx
is a poor name and the variable itself is kind of useless. Take advantage of the Return
statement.
Try
If inArray Is Nothing Then
Return False
ElseIf inArray.Length <= 0 Then
Return False
ElseIf inArray(0) Is Nothing Then
Return False
End If
Catch ex As Exception
Return False
End Try
Return True
Of course, you could take this a step further by doing away with the Elifs.
Try
If inArray Is Nothing Then Return False
If inArray.Length <= 0 Then Return False
If inArray(0) Is Nothing Then Return False
Catch ex As Exception
Return False
End Try
Return True
Which leads us to using some short circuiting.
Try
If inArray Is Nothing
OrElse inArray.Length <= 0
OrElse inArray(0) Is Nothing Then
Return False
End If
Catch ex As Exception
Return False
End Try
Return True
Then flip the condition so we can directly return.
Try
Return (inArray Is Not Nothing
AndAlso inArray.Length > 0
AndAlso inArray(0) Is Not Nothing)
Catch ex As Exception
Return False
End Try
-
\$\begingroup\$ Thanks RubberDuck. I just started playing in VB.Net so not really sure what exceptions to expect yet, hence the generic Catch for all exceptions. I was planning to build more specific ones as I debug. I don't know if that is good practice, but aside from experience I don't know how you would anticipate exceptions. \$\endgroup\$CBRF23– CBRF232015年06月30日 14:47:32 +00:00Commented Jun 30, 2015 at 14:47
-
1\$\begingroup\$ Okay, so what I'm going to recommend may not be best practice, but it has worked for me. Don't catch any exceptions at all until you find a bug. When an exception gets thrown that crashes your program, you've now added an exception to the list that needs to be caught and handled. Your mileage may vary. The docs are also a good source of information on what exceptions we might expect. \$\endgroup\$RubberDuck– RubberDuck2015年06月30日 14:50:48 +00:00Commented Jun 30, 2015 at 14:50
-
\$\begingroup\$ The reason I went with
elseIf
is because each condition needs to be checked in that order and if one is true, then we need to stop evaluating to not throw an exception. e.g. ifinArray is Nothing=True
then trying to checkInArray.Length
will throw an exception because the array is uninitialized. I'm afraid theOrElse
andAndAlso
examples will attempt to evaluate all conditions and result in an exception being thrown. When you use theReturn
statement, does it also exit the function immediately? Because if so, then that seems a lot cleaner. \$\endgroup\$CBRF23– CBRF232015年06月30日 14:51:12 +00:00Commented Jun 30, 2015 at 14:51 -
\$\begingroup\$ Yes. Return immediately exits the function. One of the beautiful additions to the language. Also, your fears about AndAlso & OrElse are misplaced. They short circuit. In other words, they only evaluate as far as they need to. support.microsoft.com/en-us/kb/817250 \$\endgroup\$RubberDuck– RubberDuck2015年06月30日 14:54:30 +00:00Commented Jun 30, 2015 at 14:54
-
\$\begingroup\$ Awesome, thank you RubberDuck - very helpful link! I did look at the MSDN article on Return but it didn't explicitly state that it also exits the function - though the sample code makes it appear so. Also great to know that logical operators short-circuit - I will remember that for future designs! Excellent, excellent feedback! \$\endgroup\$CBRF23– CBRF232015年06月30日 15:05:14 +00:00Commented Jun 30, 2015 at 15:05