I have a function I have written in VB for use in SSRS 2012. It takes an integer and converts it to full text duration - for example 900 becomes "15 minutes", 9000 becomes "2 hours 30 minutes", and 7201 is "2 hours 1 second".
The code works, and is:
Public Function SecondsFullText(ByVal TotalSeconds As Integer) As String
Dim hours As Integer = new Integer()
Dim minutes As Integer = new Integer()
Dim seconds As Integer = new Integer()
Dim hourString as String = ""
Dim minuteString as String = ""
Dim secondString as String = ""
hours = floor(TotalSeconds / 3600)
minutes = floor((TotalSeconds mod 3600) / 60)
seconds = (TotalSeconds mod 3600) mod 60
If hours = 1 Then
hourString = Cstr(hours) & " hour"
Else If hours > 1 Then
hourString = Cstr(hours) & " hours"
End If
If minutes = 1 Then
minuteString = Cstr(minutes) & " minute"
Else If minutes > 1 Then
minuteString = Cstr(minutes) & " minutes"
End If
If seconds = 1 Then
secondString = Cstr(seconds) & " second"
Else If seconds > 1 Then
secondString = Cstr(seconds) & " seconds"
End If
If hours > 0 and (minutes > 0 or seconds > 0) Then
hourString = hourString & " "
End If
If minutes > 0 and seconds > 0 Then
minuteString = minuteString & " "
End If
return hourString & minuteString & secondString
End Function
My question is: how do I make this better? It seems very clunky, with too many IFs to handle the different possibilities for single/plural, spaces, etc. I feel that this can be better, but I'm not sure how.
This is called in Report Builder 3.0 / SSRS 2012 with this expression: =Code.SecondsFullText(Parameters!Integer.Value)
-
\$\begingroup\$ It would be easier to provide a review and answer if the function that called this or displayed the resulting string was included as well. \$\endgroup\$pacmaninbw– pacmaninbw ♦2019年07月03日 14:17:14 +00:00Commented Jul 3, 2019 at 14:17
-
\$\begingroup\$ @pacmaninbw Noted, and updated. There's no real function that calls it as it's for use in a report in SSRS. \$\endgroup\$BishNaboB– BishNaboB2019年07月03日 14:38:37 +00:00Commented Jul 3, 2019 at 14:38
-
\$\begingroup\$ I did this as a one-liner in groovy for fun--converted milliseconds to something like 1d2h3m10s. I don't think I have the one-liner any more but I converted it into 4 significantly more readable lines you're welcome to if you want them :) (Now it's part of Java so I don't really need it any more, you can have it) \$\endgroup\$Bill K– Bill K2019年07月03日 22:10:29 +00:00Commented Jul 3, 2019 at 22:10
-
2\$\begingroup\$ Code aside, "an integer" has no units and cannot be converted to a string representation of time with additionally specifying units. So you don't just have an integer; you have an integer number of seconds. You need to be more careful about this in your documentation and communication. \$\endgroup\$jpmc26– jpmc262019年07月03日 23:02:26 +00:00Commented Jul 3, 2019 at 23:02
-
\$\begingroup\$ @jpmc26 That's an interesting concept to consider. In this scenario, this is a value from a SQL query which is essentially "time allowed in seconds". I could have done this in SQL, but wanted to A) see if I could do it report side and B) learn different ways of looking at it. It's certainly helped my coding knowledge and the way I look at code - especially the breakdown provided by RickDavin \$\endgroup\$BishNaboB– BishNaboB2019年07月04日 05:36:16 +00:00Commented Jul 4, 2019 at 5:36
3 Answers 3
You should look into the TimeSpan
struct, which provides a nice method TimeSpan.FromSeconds()
.
Having filled a TimeSpan
struct by e.g calling the FromSeconds()
method, you can just access its properties Seconds
, Minutes
and Hours
.
Instead of concating strings like you do, you should consider to use a StringBuilder
.
To get rid of the If ... Else If
you can just use the singular and only if the value is greater than 1 you add a s
to the string/StringBuilder.
Based on the .NET Naming Guidelines method parameters should be named using camelCase
casing hence TotalSeconds
should be totalSeconds
Implementing the mentioned points will look like so
Public Function SecondsFullText(ByVal totalSeconds As Double) As String
Dim timeSpan As TimeSpan = TimeSpan.FromSeconds(totalSeconds)
Dim stringBuilder As StringBuilder = New StringBuilder()
Dim hours As Integer = timeSpan.Hours
Dim minutes As Integer = timeSpan.Minutes
Dim seconds As Integer = timeSpan.Seconds
stringBuilder.Append(ToText(hours, "hour"))
If hours > 0 AndAlso (minutes > 0 OrElse seconds > 0) Then
stringBuilder.Append(" ")
End If
stringBuilder.Append(ToText(minutes, "minute"))
If minutes > 0 AndAlso seconds > 0 Then
stringBuilder.Append(" ")
End If
stringBuilder.Append(ToText(seconds, "second"))
Return stringBuilder.ToString()
End Function
Private Function ToText(value As Integer, singularName As String) As String
If value = 0 Then Return String.Empty
If value = 1 Then Return String.Format("{0} {1}", value, singularName)
Return String.Format("{0} {1}s", value, singularName)
End Function
-
\$\begingroup\$ Thank you for this. Unfortunately it doesn't look as though StringBuilder is valid in the VB implementation in SSRS/Report Builder 3.0. It returns this error: There is an error on line 46 of custom code: [BC30002] Type 'StringBuilder' is not defined. \$\endgroup\$BishNaboB– BishNaboB2019年07月03日 14:36:22 +00:00Commented Jul 3, 2019 at 14:36
-
1\$\begingroup\$ You need to add an import to System. Text \$\endgroup\$Heslacher– Heslacher2019年07月03日 14:39:25 +00:00Commented Jul 3, 2019 at 14:39
-
\$\begingroup\$ How does this handle a duration greater than 24 hours? \$\endgroup\$BishNaboB– BishNaboB2019年07月03日 15:17:25 +00:00Commented Jul 3, 2019 at 15:17
-
1\$\begingroup\$ Then you need to query the
TotalHours
property \$\endgroup\$Heslacher– Heslacher2019年07月03日 15:18:25 +00:00Commented Jul 3, 2019 at 15:18 -
\$\begingroup\$ Thank you. I had to change
Dim stringBuilder As StringBuilder = New StringBuilder()
toDim stringBuilder As New System.Text.StringBuilder()
to make it work in SSRS. \$\endgroup\$BishNaboB– BishNaboB2019年07月03日 15:23:29 +00:00Commented Jul 3, 2019 at 15:23
As @Hesclacher has given a fantastic answer, which is not only one that I have upvoted but I would personally mark it as the correct answer if I had the power, I am hesitant to improve upon it. But I would like to offer constructive feedback on your original code.
I have a personal distinction between being a VB Coder and a .NET Developer. I think you can use VB.NET and be what I define to be a .NET Developer - that is you are making traditional .NET calls like any C# dev would use. I define a VB Coder has someone who still thinks and codes with VBA in mind, and such a person relies upon some of the compatibility calls, such as CStr.
According to my own such definitions, a VB Coder would use CStr whereas a .NET Developer would use String.Format (as did Heschalcher) or better yet, Interpolated Strings.
VB Coder:
hourString = Cstr(hours) & " hours"
.NET Developer:
hourString = String.Format("{0} hours", hours)
Or
hourString = $"{hours} hours"
To calculate the number of hours, if you aren't going to use something nice like TimeSpan, I would change this line of code:
hours = floor(TotalSeconds / 3600)
To simply use integer division:
hours = TotalSeconds \ 3600
Also, VB.NET does support short-circuited conditionals (unlike VBA), so this line of code:
If hours > 0 and (minutes > 0 or seconds > 0) Then
could be changed to:
If hours > 0 AndAlso (minutes > 0 OrElse seconds > 0) Then
This example employs short-circuiting with the AndAlso and OrElse operators.
Again, Heschachler's answer with TimeSpan
and StringBuilder
is spot-on.
-
\$\begingroup\$ Hi Rick. Thank you for the distinction between coder types, it's an interesting observation. I suppose the
cstr()
comes from the fact this is code added to a report in SSRS, which uses those functions. Short circuiting is something I use in SSRS, and forgot to add in to the code example. Integer division is not something I'm overly familiar with so gives me something to read up on. \$\endgroup\$BishNaboB– BishNaboB2019年07月03日 20:31:35 +00:00Commented Jul 3, 2019 at 20:31
Building on @RickDavin.
VB.Net also has a robust If()
function that shortcuts the checks (unlike VBA's IIf()
function)
If hours = 1 Then
hourString = Cstr(hours) & " hour"
Else If hours > 1 Then
hourString = Cstr(hours) & " hours"
End If
Becomes
hourstring = If(hours = 1, "hour", "hours")
@Heslacher's function can then be:
Private Function ToText(value As Integer, singularName As String) As String
Return If(value = 0, String.Empty, String.Format("{0} {1}{2}", value, singularName, If(value=1,"","s")))
End Function
Note, I am still in the VB coder camp, and my kludge to the string builder format above is based some limited practice.