more elegant way than this

[Edward W.]

hello, I have this function below which is simple and easy to understand

private function ListHeight (byval UserScreenHeight as int) as int
if UserScreenHeight < 1024
return 30
else
return 50
end if
end function

It is very functional but seems pretty lame to me, like I could do better.
is there a more "professional" way or "elegant" way to do this?

Ed

 

[Ged Mead]

How about:

Private Function ListHeight(ByVal UserScreenHeight As Integer) As Integer

Return IIf(UserScreenHeight < 1024, 30, 50)

End Function

BTW you can't use Int in VB.Net. I thas to be integer or Int16, etc

Ged

 

[Jay B. Harlow [MVP - Outlook]]

Edward,

hello, I have this function below which is simple and easy to understand
It is very functional

Personally those are attributes that all methods should strive for!

is there a more "professional" way or "elegant" way to do this?

Only thing I would consider changing is the name of the function, normally I
name functions & sub as action statements (verb or verb noun), where as I
normally name Properties as a noun.

So I would name the function GetListHeight, as a ListHeight function
suggests that I am listing (List as in verb) Height (noun), if I were
listing Height, I would probably make it plural so it reads better...

Hope this helps
Jay

 

[Jay B. Harlow [MVP - Outlook]]

Ged,
Try adding Option Strict On to the source file, you will receive compile
errors.

As IIf expects object parameters & returns an object, you would need a
DirectCast, CType, or CInt around the IIf to cast the value returned back to
Integer.

Hope this helps
Jay

 

[Charles Law]

Hi Edward

In addition to the other answers, I prefer to have only one exit point. Thus

<code>
Private Function GetListHeight (ByVal userScreenHeight As Integer) As
Integer

Dim height As Integer

If userScreenHeight < 1024 Then
height = 30
Else
height = 50
End If

Return height

End Function
</code>

HTH

Charles

 

[Ged Mead]

Thanks, Jay.

Good point ;-} . I hadn't spotted that.

Ged

 

[Cor Ligthert]

Edward,

You make the same error as I always do, I wanted they made that "then" not
needed anymore, it does nothing. I would keep it this way you do, however
possible is as well

private function GiveListHeight (byval UserScreenHeight as integer) as
integer
if UserScreenHeight < 1024 then return 30
return 50
end function

However I never do that like this and would take your way.

Cor


Cor

 

[Cor Ligthert]

Or more likely Charles

 

[Jay B. Harlow [MVP - Outlook]]

Cor,
How about:

private function GiveListHeight (byval UserScreenHeight as integer) as
integer
if UserScreenHeight < 1024 then return 30 else return 50
end function

Which is Ged's example without needing DirectCast.


I will use a "guard statement" (similar to what you have) when there is more
logic in the Else block over the Then block. Or when validating parameters:

If UserScreenHeight < 0 Then Throw New
ArgumentOutOfRangeException("UserScreenHeight", UserScreenHeight, "User
Screen Height cannot be Negative.")

Although I'm not sure if this function needs to validate its parameter...

Just a thought
Jay

 

[Larry Serflaten]

hello, I have this function below which is simple and easy to understand

It is very functional but seems pretty lame to me, like I could do better.
is there a more "professional" way or "elegant" way to do this?

No one has mentioned this style yet:

Private Function ListHeight (Byval UserScreenHeight As Integer) As Integer
ListHeight = 50
If UserScreenHeight < 1024 Then ListHeight = 30
End Function


LFS

 

[Jay B. Harlow [MVP - Outlook]]

Edward,
<me too>
I just wanted to add you can also use Select Case:

Private Function GetListHeight(ByVal userScreenHeight As Integer) As
Integer
Select Case userScreenHeight
Case Is < 1024
Return 30
Case Else
Return 50
End Select
End Function

</me too>

I've been at shops that prefer Select Case over If, although I really don't
see what it buys you in the above...

I could see using Select Case if you had multiple return values.

Select Case userScreenHeight
Case Is < 0
Throw New ArgumentOutOfRangeException("UserScreenHeight",
userScreenHeight, "User Screen Height cannot be Negative.")
Case Is < 800
Return 25
Case Is < 1024
Return 30
Case Else
Return 50
End Select

Of course you can do variations on the above...

Just a thought
Jay

 

[Herfried K. Wagner [MVP]]

private function GiveListHeight (byval UserScreenHeight as integer) as
integer
if UserScreenHeight < 1024 then return 30
return 50
end function

Well, I prefer the original solution or the 'Select Case' solution too,
because it's easier to extend and understand.

 

[Robin Tucker]

I like this one the best. We often used to have geeky debates over a!=0?b:c
in C++. The conclusion was that if it doesn't buy you anything (in terms of
performance in a performance critical piece of code) then prefer the more
readable solution, which is not neccessarily the one with greatest brevity.

 

[Jay B. Harlow [MVP - Outlook]]

Robin,
Interesting: I find the original multi-line If-Then-Else to be more readable
then the select case. As I don't see the select case is buying you anything
more (then the multi-line If-Then-Else).

If at a later date I needed to add "conditions" to the If-Then-Else I would
either use a ElseIf or change it to a Select Case at that time... FWIW:
Changing it to a Select Case is the Substitute Algorithm refactoring:
http://www.refactoring.com/catalog/substituteAlgorithm.html

I agree rarely in C++ would I use the ternary operator (a!=0?b:c )

I also agree on the comments about performance, as I only worry about
performance when that routine was proven to be a performance problem via
profiling.

Just a thought
Jay