2
\$\begingroup\$

A loan calculator has to be created for the proNetBank. Method of processing the loan is as follows:

Vehicle loan is given for a period of three to seven years depending on the customers preference. the current interest rate is 20% per annum. The minimum loan amount is lkr 500,000 and the maximum loan amount is 7,000,000 , in addition maximum that could be obtained also depends on the category of the vehicle as below.

  • Brand new or unregistered vehicles. Up to 100% of the vehicle value -vehicles within 6 years of 1st registration. Up to 85% of vehicle value hybrid brand new and unregistered vehicles within 2 years of manufacturing. Up to 85% of the vehicle value.The application should obtain the necessary inputs using appropriate controls and calculate and display the installment value.

So I wrote my code:

Private Sub RadioButton2_CheckedChanged(sender As Object, e As EventArgs) Handles rbVeh5yrs.CheckedChanged
    If rbVeh5yrs.Checked = True Then
        txtBorrow.Visible = True
        lblBorrowingAmount.Text = "Enter the amount you want to borrow"
    Else
        txtBorrow.Visible = False
        lblBorrowingAmount.Text = ""
    End If


End Sub

Private Sub btnCalculate_Click(sender As Object, e As EventArgs) Handles btnCalculate.Click

    If txtBorrow.Text = "" Then
        MessageBox.Show("Pls Enter the amount you want to borrow")
    Else
        intBorrow = txtBorrow.Text
        If intBorrow < 500000 Then
            MessageBox.Show("Lowest Amount which can be borrow is RS.500000")
        ElseIf intBorrow > 7000000 Then
            MessageBox.Show("Maximum amount which can be borrowed is RS.7000000")
        End If
    End If


    intBorrow = txtBorrow.Text
    If rbVeh5yrs.Checked = True Then
        decVehicle = intBorrow * 0.8
        decInterestPay = (decVehicle + (decVehicle * 0.2 * decYear))
        decInstallment = decInterestPay / decMonthly
        lblMonthlyPayment.Text = "Monthly Installments " & decInstallment
    End If


    If rbBN.Checked = True Then
        decVehicle = intBorrow
        decInterestPay = (decVehicle + (decVehicle * 0.2 * decYear))
        decInstallment = decInterestPay / decMonthly
        lblMonthlyPayment.Text = "Monthly Installments " & decInstallment

    End If

    If rbHybrid.Checked = True Then
        decVehicle = intBorrow * 0.8
        decInterestPay = (decVehicle + (decVehicle * 0.2 * decYear))
        decInstallment = decInterestPay / decMonthly
        lblMonthlyPayment.Text = "Monthly Installments " & decInstallment
    End If



End Sub

Private Sub RadioButton1_CheckedChanged(sender As Object, e As EventArgs) Handles rbBN.CheckedChanged
    If rbBN.Checked = True Then
        txtBorrow.Visible = True
        lblBorrowingAmount.Text = " Enter the amount you want to borrow "
    Else

        txtBorrow.Visible = False
        lblBorrowingAmount.Text = ""

    End If




End Sub

Private Sub rbHybrid_CheckedChanged(sender As Object, e As EventArgs) Handles rbHybrid.CheckedChanged
    If rbHybrid.Checked = True Then
        txtBorrow.Visible = True
        lblBorrowingAmount.Text = "Enter the amount you want to borrow"
    Else
        txtBorrow.Visible = False
        lblBorrowingAmount.Text = ""

    End If


End Sub

Private Sub btnExit_Click(sender As Object, e As EventArgs) Handles btnExit.Click
    Close()
End Sub

Private Sub rb3Years_CheckedChanged(sender As Object, e As EventArgs) Handles rb3Years.CheckedChanged
    If rb3Years.Checked = True Then
        decYear = 3
        decMonthly = 36
    End If
End Sub

Private Sub rb4Years_CheckedChanged(sender As Object, e As EventArgs) Handles rb4Years.CheckedChanged
    If rb4Years.Checked = True Then
        decYear = 4
        decMonthly = 36
    End If
End Sub

Private Sub rb5Years_CheckedChanged(sender As Object, e As EventArgs) Handles rb5Years.CheckedChanged
    If rb5Years.Checked = True Then
        decYear = 5
        decMonthly = 48
    End If
End Sub

Private Sub rb6Years_CheckedChanged(sender As Object, e As EventArgs) Handles rb6Years.CheckedChanged
    If rb6Years.Checked = True Then
        decYear = 6
        decMonthly = 60
    End If
End Sub

Private Sub rb7Years_CheckedChanged(sender As Object, e As EventArgs) Handles rb7Years.CheckedChanged
    If rb7Years.Checked = True Then
        decYear = 7
        decMonthly = 72
    End If
End Sub

Can you check it and tell me whether it is good enough? If there is room for improvement, please let me know.

\$\endgroup\$
  • 1
    \$\begingroup\$ Is this a very simplified way to calculate interest that you are told to use? That's not how interest rates works in real situations. Is this an assignment? If so, are you seriously encouraged to use hungarian notation? \$\endgroup\$ – Guffa Nov 14 '14 at 20:46
  • \$\begingroup\$ @guffa yes this is an assignment and i was given a fixed interest rate of 20% so what I have done is right? \$\endgroup\$ – Shihaan Nov 14 '14 at 20:59
  • \$\begingroup\$ @guffa yes thats what our teacher expects from us \$\endgroup\$ – Shihaan Nov 14 '14 at 21:02
  • 1
    \$\begingroup\$ Fire the teacher. Burn the Hungarian notation. \$\endgroup\$ – RubberDuck Nov 14 '14 at 21:11
  • \$\begingroup\$ awh thnx man, just will pass the assignments with these, but wont use them once i start working, is my code correct? does it need any amendments? i mean have i declared the variables right??? \$\endgroup\$ – Shihaan Nov 14 '14 at 21:13
1
\$\begingroup\$

I'm going to focus my review on just one Sub.

btnCalculate_Click

  • First off, I understand that your teacher wants you to use Hungarian notation, but out in the real world, this makes us cringe. Don't use it outside of class, unless you plan on doing it right.

  • You have a typo right here. Use "Please" instead of "Pls".

    MessageBox.Show("Pls Enter the amount you want to borrow")
    
  • And a potential bug soon after.

    intBorrow = txtBorrow.Text  
    

    You can't rely on a user to type in an Integer. You should use Int32.TryParse here instead. For example:

    If Int32.TryParse(txtBorrow.Text, intBorrow) Then
        ' current logic
    Else
        MessageBox.Show("You must enter an integer value to borrow.")
        Exit Sub
    End If
    
  • You have more typos. The two strings you're showing to the user should be consistent.

    If intBorrow < 500000 Then
        MessageBox.Show("Lowest Amount which can be borrow is RS.500000")
    ElseIf intBorrow > 7000000 Then
        MessageBox.Show("Maximum amount which can be borrowed is RS.7000000")
    End If
    

    Also, consider what happens if the lowest amount and/or highest amount need to change. You would have to change it in two places. It's better to extract some constants so you only have to change it once. You should also look up String.Format.

    Const lowestAmount As Integer = 50000
    Const highestAmount As Integer = 7000000
    Dim message As String = "{0} amount which can be borrowed is RS.{1}"
    
    If intBorrow < lowestAmount Then
        MessageBox.Show(String.Format(message,"Lowest",lowestAmount))
    ElseIf intBorrow > hightestAmount Then
        MessageBox.Show(String.Format(message,"Highest",HighestAmount))
    End If
    

    Lastly, I recommend extracting all of that logic into it's own subroutine in order to de-clutter the Click handler.

  • I had to look really hard to find the difference between the next three If statements. It turns out that the only thing that changes is what you set decVechicle to. Given that, don't repeat yourself. The rest of that code gets executed no matter what, so remove it from the inside of the If block.

    If rbVeh5yrs.Checked OrElse rbHybrid.Checked Then
        decVehicle = intBorrow * 0.8
    Else rbBN.Checked Then
        decVehicle = intBorrow
    End If
    
    decInterestPay = (decVehicle + (decVechicle * 0.2 * decYear))
    '.... etc
    

    I also want you to take note of two things.

    1. There's no need to say If rbVeh5yrs.Checked = True. When you do that, what you're really saying is If True = True (or alternatively, If False = True).
    2. You also take the same action whether rbVeh5yrs or rbHybrid is Checked. So, I used the OrElse Operator. We could also switch the order up a little bit and check rbBN.Checked first, and then catch the others in the Else part of the If block.

      If rbBN.Checked Then
          decVehicle = intBorrow
      Else
          decVechicle = intBorrow * 0.8
      End If
      
      decInterestPay = (decVehicle + (decVechicle * 0.2 * decYear))
      '.... etc
      
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ would it be easier if i did these using a function? I still feel this could be done better \$\endgroup\$ – Shihaan Nov 14 '14 at 21:53
  • \$\begingroup\$ Yes! By all means yes! Extract functions! Normally I would scold you for having the logic so tightly bound to your form, but I figured you're taking classes and haven't been doing this long so I took it easy on you. ++ for nice formatting though. I forgot to mention it. \$\endgroup\$ – RubberDuck Nov 14 '14 at 21:55
  • \$\begingroup\$ thanks man . thank you very much, can you show me an example of how i could use a function to do one part? if you show me one i'll do the rest and post you the code only if you can \$\endgroup\$ – Shihaan Nov 14 '14 at 22:03
  • \$\begingroup\$ what does this mean? "Dim message As String = "{0} amount which can be borrowed is RS.{1}"" I have not learnt that \$\endgroup\$ – Shihaan Nov 14 '14 at 22:05
  • \$\begingroup\$ {0} and {1} stand in for the arguments that you pass into String.Format(). I left a link in my answer that will take you to some examples. \$\endgroup\$ – RubberDuck Nov 14 '14 at 22:13

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.