2
\$\begingroup\$

I had to write a function that calculates the percentage of values in table less or equal to 2. Is my code right or is there anything missing?

Public Function intPercentage As Integer

 Dim intAge( , ) As Integer = {{0, 2, 5, 10}

 {2, 0, 1, 3}

 {5, 1, 0, 6}

 {10, 3, 6, 0}}

Dim intRow, intCol As Integer

For intRow= 0 to 3

 For intCol = 0 to 3

 If intAge(intRow, intCol) >= 2 Then

 intTotal = intTotal + intAge(intRow, intCol)

 Next intCol

   Next intRow

 intPercentage = intTotal / 100

 End Function
\$\endgroup\$
  • \$\begingroup\$ Does it work for all test cases you can imagine? What test cases did you imagine? \$\endgroup\$ – Jeroen Vannevel Oct 8 '14 at 17:09
  • \$\begingroup\$ @JeroenVannevel the calculation? yes the calculation works fine afaik, i'm more interested in finding out of anyone could pick out anything wrong with my code, just a second opinion \$\endgroup\$ – shaka Oct 8 '14 at 17:16
  • 4
    \$\begingroup\$ This question confuses me. The description says "calculates the percentage of values in table less or equal to 2" but the code says: If intAge(intRow, intCol) >= 2 Then intTotal = intTotal + intAge(intRow, intCol). Two things, 1. are you looking for ages less than 2, or greater, and 2. are you averaging their ages, or getting the % of ages above/belop the 2 threshold? \$\endgroup\$ – rolfl Oct 8 '14 at 18:17
3
\$\begingroup\$

Naming

Kill the Hungarian Notation. Kill it with fire. Don't bother taking it out back to bury it.

kill it with fire

Hungarian notation can be done right, but using it to tell yourself what datatype a variable is is not doing it right. Even if you must just insist on using hungarian notation this would not be right.

Dim intAge( , ) As Integer = {{0, 2, 5, 10}

intAge is not an integer. It is an array of arrays of integers.

Functions/Subs should have Verb-Noun style names. They should also be PascalCased.

Indentation

It's important. Like, really really important. Proper indentation makes it orders of magnitude easier to read code. Considering code will be read many more times than it will be written, it's important to make sure people can read it.

Everything inside of Function...End Function should be indented one level. Everything inside of For...Next should be indented one more level. Same goes for If...End If blocks.

Getting Down to Business

  • A percentage is a double, not an integer.

    Public Function intPercentage As Integer
    

    If you want an integer representation of the percentage, you should probably leave a comment explaining why.

  • A function isn't much good if it will return the same results every time. This one will because you've hardcoded the array of arrays into the function. It would be much more useful to accept a 2-dimensional array as an argument instead.

  • I'm not personally a fan of declaring multiple variables on a single line, but there's technically nothing wrong with it.

    Dim intRow, intCol As Integer
    
  • Since we're now going to accept an array as an argument, we'll no longer be able to hardcode For row = 0 to 3. You'll need to call GetLength on the internal array and Length on the outer one. (Example will be below.)

  • You never declared intTotal. (Which you're going to call totalAge now, right?)

  • Don't skimp on the End Ifs. Always be sure to close the If block unless you really write a one liner. (I don't recommend one liners by the way.)

  • Prefer the Return keyword over assigning values to the function name.

  • You can't just divide a number by 100 and call it a percentage.

    intPercentage = intTotal / 100
    

    In order to get the results you're really after, you need to divide the count of ages 2 or older by the count of all ages, then multiply by 100.

I took the liberty of re-writing the code.

Public Function GetPercentage(ages(,) As Integer) As Double

    Dim row As Integer
    Dim col As Integer
    Dim overTwo As Integer
    Dim total As Integer

    For row = 0 to ages.Length - 1
        total = total + ages.GetLength(row)

        For col = 0 to ages.GetLength(row) - 1
            If ages(row, col) >= 2 Then
                overTwo = overTwo + 1
            End If
        Next col
    Next row

    Return (overTwo / total) * 100
End Function
\$\endgroup\$
  • \$\begingroup\$ You're welcome. Out of curiousity, what exactly are you trying to return from this function? \$\endgroup\$ – RubberDuck Oct 9 '14 at 9:43
  • \$\begingroup\$ I had to get the percentage of values in table less or equal to 2 \$\endgroup\$ – shaka Oct 9 '14 at 15:12
  • \$\begingroup\$ @shaka compared to the total number of entries in the table, correct? \$\endgroup\$ – RubberDuck Oct 9 '14 at 15:23
  • \$\begingroup\$ Yeah basically that \$\endgroup\$ – shaka Oct 9 '14 at 16:46
  • \$\begingroup\$ I updated my answer accordingly. \$\endgroup\$ – RubberDuck Oct 9 '14 at 17:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.