0
\$\begingroup\$

My code works just fine but it seems like I have too many variables which could confuse people from what the code does. Any ideas? Or does this look fine. Feel free to add any other tips to make my code more efficient.

This subroutine is for a button which when clicked rotates a players position. (The overall program is a rotation management application for football)

Private Sub btnConfirmRotation_Click(sender As Object, e As EventArgs) Handles btnConfirmRotation.Click
        
    If comPlayersToBeRotated.SelectedIndex < 0 Then 'If there are no players to be rotated the button does nothing
        Exit Sub
    End If

    Dim lineIndex As Integer = alreadyAdded(comPlayersToBeRotated.SelectedIndex) 'Gets the line index in the text file for the players details
    Dim allPlayers As String() = System.IO.File.ReadAllLines(myFile) 'List of all details
    Dim entireFile As String = System.IO.File.ReadAllText(myFile) 'String of entire file

    Dim playerDetails As String() = allPlayers(lineIndex).Split("|") 'Gets the details of the selected player in the combo box
    Dim positionOfPlayerRotating As String = playerDetails(3) 'Assigns position portion of array
    Dim rotationOfPlayerRotating As String = playerDetails(2) 'Assigns jersey of person they are rotating with portion of array

    For Each line In allPlayers
        Dim thisPlayersDetails As String() = line.Split("|") 'The line its on is split into each section of the details
        Dim positionOfThisPlayer As String = thisPlayersDetails(3)
        Dim jerseyOfThisPlayer As String = thisPlayersDetails(1)
        Dim temp As String

        If rotationOfPlayerRotating = jerseyOfThisPlayer Then

            'Swaps "position"
            temp = thisPlayersDetails(3)
            thisPlayersDetails(3) = positionOfPlayerRotating
            playerDetails(3) = temp

            'Rewrites file
            entireFile = entireFile.Replace(line, String.Join("|", thisPlayersDetails))
            entireFile = entireFile.Replace(allPlayers(lineIndex), String.Join("|", playerDetails))

            System.IO.File.WriteAllText(myFile, entireFile) 'Actually writes to file

            'Sets to default if items are above zero so i dont get an out of range exception
            If comPlayersToBeRotated.Items.Count > 0 Then
                comPlayersToBeRotated.SelectedIndex = 0
            End If

            If lbxPlayers.SelectedIndex >= 0 Then 'If an item is selected
                ShowDetails() 'Shows the detail of that player, even if its not the player that rotated so when it is, the details update
            End If
        End If
    Next

    'Removes that player from the combo box and adds to already rotated so it won't keep rotating the first player
    alreadyAdded.RemoveAt(comPlayersToBeRotated.SelectedIndex)
    alreadyRotated.Add(lineIndex)
    comPlayersToBeRotated.Items.RemoveAt(comPlayersToBeRotated.SelectedIndex)
End Sub
\$\endgroup\$
1
\$\begingroup\$

Sometimes you are in the need for a lot of variables. The best way to make your code not confusing for others is to name the variables meaningful and easy to distinguish.

For example the variables

Dim positionOfPlayerRotating As String
Dim rotationOfPlayerRotating As String

are difficult to distinguish.

That beeing said, there would be another possibility, use classes.

How about adding a class Player with properties Position, Jersey, Rotation and whatever properties you need.
Next having a Dim players as List(Of Player) which can be loaded from your file myFile. Doing this at the start of your application you wouldn't need to load it every time you are clicking this button or in other methods where you need the players.
Now you could have e.g

Dim rotatingPlayer as Player = ... retrieve e.g from combobox comPlayersToBeRotated
For Each player As Player in players
    If rotatingPlayer.Rotation = player.Jersey Then  
        ' Do what has to be done
    End If
Next

which would be more readable without having the need for that much variables.
You could also use Linq to "filter" the players like so

For Each player As Player in players.Where(Function(p) p.Jersey = rotatingPlayer.Rotation)  
    ' Do what has to be done
Next

which would only iterate over the players which Jersey property equals the Rotation property of the rotatingPlayer.

Saving could take place either by having a "save-button" or at the end of the application. In your current method you are saving the players each time the condition rotationOfPlayerRotating = jerseyOfThisPlayer is true.

Althought I come from a VB background you may assume that the provided code maybe isn't correct because I have switched to C# some years ago.

\$\endgroup\$

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.