2
\$\begingroup\$

Is there any way I can cut down on code? I'm new to this.

Sub addtest()


    Dim decision As Char
    Dim fullline As String = " "

    Dim word(9), def(9), minorerror1(49), minorerror2(49), minorerror3(49), minorerror4(49), minorerror5(49) As String

    Console.Clear()
    Console.WriteLine("Please select the year of students you wish to save the test for: ")
    Console.WriteLine()
    Console.WriteLine("3 - Year 3")
    Console.WriteLine("4 - Year 4")
    Console.WriteLine("5 - Year 5")
    Console.WriteLine("6 - Year 6")


    Console.WriteLine()


    Dim testyear As Integer = Console.ReadLine()

    Select Case testyear

        Case 1
        Case 2
        Case 3
        Case 4

    End Select

    Console.WriteLine("Please enter 10 of your selected words, defenitions and selected amount minor errors to be featured in the following test")

    FileOpen(5, "F:\Computing\Spelling Bee\testtests.csv", OpenMode.Append)

    Console.Clear()

    Do

        counter = counter + 1
        Console.Write("Word: ")
        word(counter) = Console.ReadLine
        Console.Write("Defenition: ")
        def(counter) = Console.ReadLine

        Console.WriteLine("Type in a max of 5 minor errors")


        Console.Write("Minor error 1: ")
        minorerror1(counter) = Console.ReadLine
        Console.Write("Minor error 2: ")
        minorerror2(counter) = Console.ReadLine
        Console.Write("Minor error 3: ")
        minorerror3(counter) = Console.ReadLine
        Console.Write("Minor error 4: ")
        minorerror4(counter) = Console.ReadLine
        Console.Write("Minor error 5: ")
        minorerror5(counter) = Console.ReadLine


    Loop Until counter = 9
    Console.Clear()




    Console.WriteLine("Are you sure you want to save this test? (y/n) ")
    decision = Console.ReadLine
    If decision = "y" Or decision = "Y" Then
        For counter As Integer = 0 To 9
            fullline = testyear & "," & word(counter) & "," & def(counter) & "," & minorerror1(counter) & "," & minorerror2(counter) & "," & minorerror3(counter) & "," & minorerror4(counter) & "," & minorerror5(counter)
            PrintLine(5, fullline)
            Console.WriteLine("Word: " & (word(counter.ToString)) & " Defenition: " & (def(counter.ToString)) & " Minor error 1: " & (minorerror1(counter.ToString)) & " Minor error 2: " & (minorerror2(counter.ToString)) & " Minor error 3: " & (minorerror3(counter.ToString)) & " Minor error 4: " & (minorerror4(counter.ToString)) & " Minor error 5: " & (minorerror5(counter.ToString)))
        Next
        FileClose(5)
        Console.Clear()
        staffmenu()
    ElseIf decision = "n" Or decision = "N" Then
        addtest()

    End If

End Sub
\$\endgroup\$
  • \$\begingroup\$ The obvious would be to ommit that case structure that appears to be unused \$\endgroup\$ – SaggingRufus Jan 6 '14 at 12:04
  • \$\begingroup\$ Also instead in Console.WriteLine() you could use: Console.WriteLine(vbCrLf & "Please select the year of students you wish to save the test for: " & vbCrLf) \$\endgroup\$ – SaggingRufus Jan 6 '14 at 12:25
3
\$\begingroup\$

You should read up on arrays :

    Console.Write("Minor error 1: ")
    minorerror1(counter) = Console.ReadLine
    Console.Write("Minor error 2: ")
    minorerror2(counter) = Console.ReadLine
    Console.Write("Minor error 3: ")
    minorerror3(counter) = Console.ReadLine
    Console.Write("Minor error 4: ")
    minorerror4(counter) = Console.ReadLine
    Console.Write("Minor error 5: ")
    minorerror5(counter) = Console.ReadLine

This should be a loop which writes to an array of strings.

Proposed reading : MSDN link on arrays

\$\endgroup\$
2
\$\begingroup\$

Two things you should probably consider, classes and lists.

When the collections are going to be of variable length, it's usually more efficient on resources to use a list instead of an array

When you have items that are related it makes sense to encapsulate them into a structure such as a class.

Here's an example that shows both:

Class TestItem
    Private _word As String = ""
    Public Property Word As String
        Get
            Return _word
        End Get
        Set(value As String)
            _word = value
        End Set
    End Property
    Private _def As String = ""
    Public Property Def As String
        Get
            Return _def
        End Get
        Set(value As String)
            _def = value
        End Set
    End Property
    Public ReadOnly MinorErrors As New List(Of String)
    Public Sub New()

    End Sub
    'To get the fomratted output of each TestItem just call it's ToString method with
    ' a True for .csv format or a False for console display
    Public Overloads Function ToString(csv As Boolean) As String
        Dim retval As New System.Text.StringBuilder
        If csv Then
            retval.Append(_word & "," & _def)
            For Each item As String In MinorErrors
                retval.Append("," & item)
            Next
        Else
            retval.Append("Word: " & _word & " Defenition: " & _def)
            For I = 0 To MinorErrors.Count - 1
                retval.Append(" Minor error " & I.ToString & ": " & MinorErrors(I))
            Next
        End If
        Return retval.ToString
    End Function
End Class
Class Test
    Private _year As Integer = 0
    Public Property Year As Integer
        Get
            Return _year
        End Get
        Set(value As Integer)
            If value >= 3 AndAlso value <= 6 Then
                _year = value
            Else
                Console.WriteLine(vbNewLine & "Invalid number")
            End If
        End Set
    End Property
    Public ReadOnly TestItems As New List(Of TestItem)
    Public Sub New()

    End Sub
End Class

On the surface this might look like you're adding a lot of code, but you simplify the rest of your code tremendously. Also if in a later revision you decide to add a property or a method to a class the rest of your code isn't automatically broken.

\$\endgroup\$
0
\$\begingroup\$

Seems like your counter isn't initialized. Even if it works, you can do :

Dim counter as Integer: counter = 0


Instead of

If decision = "y" Or decision = "Y" Then

you can do something like:

If UCase(decision) = "Y" Then

At last, some people like to "type" their variables so that there is no ambiguity after all. Something like:

Dim sDecision as String
Dim iCounter as Integer

Last but not least, be sure to have all your variables declared with:

Option Explicit

At the beginning of your module

\$\endgroup\$
  • \$\begingroup\$ This is the first time I've ever seen a code review tell someone they ought to use typing in the variable name. Please don't use hungarian notation as there are much clearer ways to identify type (namely a good IDE). Read more about it here: (c2.com/cgi/wiki?HungarianNotation) \$\endgroup\$ – KyleMit Apr 10 '14 at 20:18
  • \$\begingroup\$ I didn't say the OP ought to use. I just said some coders do (and some good ones). Interesting article though. And I won't fight juste quote the article Hungarian notation inspires some of the most bitter religious wars among programmers :) \$\endgroup\$ – JMax Apr 15 '14 at 9:08

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.