7
\$\begingroup\$

Can someone review this code for me? I'm not completely sure I did this correctly. I also would like help creating the code for the "walk" "don't walk" lights.

I needed to create a traffic intersection. Each traffic light has a cycle of 31 seconds red, followed by 26 seconds green, followed by 3 seconds yellow. The red in the north-south direction starts at the same time as the green in the east-west direction. There will be a 60 second light cycle, and a 2 second overlap where the lights are red in all directions. The walk-don't walk signal reads walk for the 26 second green light, and don't walk for all other times.

Public Class _12
Private Sub N_S_G_Lights()
    pb_N.BackColor = Color.Green
    lbl_N.BackColor = Color.Green
    pb_S.BackColor = Color.Green
    lbl_S.BackColor = Color.Green
End Sub
Private Sub N_S_Y_Lights()
    pb_N.BackColor = Color.Yellow
    lbl_N.BackColor = Color.Yellow
    pb_S.BackColor = Color.Yellow
    lbl_S.BackColor = Color.Yellow
End Sub
Private Sub N_S_R_Lights()
    pb_N.BackColor = Color.Red
    lbl_N.BackColor = Color.Red
    pb_S.BackColor = Color.Red
    lbl_S.BackColor = Color.Red
End Sub
Private Sub W_E_G_Lights()
    pb_W.BackColor = Color.Green
    lbl_W.BackColor = Color.Green
    pb_E.BackColor = Color.Green
    lbl_E.BackColor = Color.Green
End Sub
Private Sub W_E_Y_Lights()
    pb_W.BackColor = Color.Yellow
    lbl_W.BackColor = Color.Yellow
    pb_E.BackColor = Color.Yellow
    lbl_E.BackColor = Color.Yellow
End Sub
Private Sub W_E_R_Lights()
    pb_W.BackColor = Color.Red
    lbl_W.BackColor = Color.Red
    pb_E.BackColor = Color.Red
    lbl_E.BackColor = Color.Red
End Sub
Private Sub _12_Load(sender As System.Object, e As System.EventArgs) Handles MyBase.Load
    pb_N.BackColor = Color.Red
    pb_E.BackColor = Color.Red
    pb_S.BackColor = Color.Red
    pb_W.BackColor = Color.Red
    lbl_N.BackColor = Color.Red
    lbl_E.BackColor = Color.Red
    lbl_S.BackColor = Color.Red
    lbl_W.BackColor = Color.Red

End Sub

Private Sub Lights()
    If lbl_time.Text <= 33 Then
        W_E_R_Lights()
    ElseIf lbl_time.Text >= 34 And lbl_time.Text <= 58 Then
        W_E_G_Lights()
    ElseIf lbl_time.Text >= 59 Then
        W_E_Y_Lights()
    ElseIf lbl_time.Text >= 60 Then
        W_E_R_Lights()
    End If
    If lbl_time.Text <= 26 Then
        N_S_G_Lights()
    ElseIf lbl_time.Text >= 27 And lbl_time.Text <= 29 Then
        N_S_Y_Lights()
    ElseIf lbl_time.Text >= 30 Then
        N_S_R_Lights()
    End If
End Sub
Private Sub Timer_N_Tick(sender As System.Object, e As System.EventArgs) Handles Timer_N.Tick
    lbl_time.Text = Val(lbl_time.Text) + 1

End Sub

Private Sub Button1_Click(sender As System.Object, e As System.EventArgs) Handles Button1.Click
    lbl_time.Text = "55"
End Sub

Private Sub Button2_Click(sender As System.Object, e As System.EventArgs) Handles Button2.Click
    lbl_time.Text = "25"
End Sub

Private Sub Timer1_Tick(sender As System.Object, e As System.EventArgs) Handles Timer1.Tick
    If lbl_time.Text <= 60 Then
        Lights()
    Else
        lbl_time.Text = "0"
    End If
End Sub
End Class
\$\endgroup\$
5
\$\begingroup\$

Naming.

N_S_G_Lights could be called NorthSouthGreenLights, but that wouldn't make it a much better name. Method names should start with a verb, they do something.

However, the pattern is consistent, which is a good thing. It's redundant though.

Structure.

The things you need to do depend on the vocabulary you're writing your code with - methods are verbs, classes are subjects. Classes are definitions for objects, which encapsulate a piece of functionality and abstract away the implementation details.

The first object I'd think of a design for, would be some TrafficLight class. I'd want the UI code to only have to be told what color that traffic light is, and draw the appropriate background color in that PictureBox.

The form code doesn't need to know about a Timer - it needs to know about a TrafficLight, and from that object it needs to know the LightColor, and it needs to be notified whenever the color changes.

I'd be expecting something like this in a class called TrafficLight:

Private Const RedSeconds As Integer = 31
Private Const GreenSeconds As Integer = 26
Private Const YellowSeconds As Integer = 3

Private LightTimer As Timer 'ticks every second

Private CurrentColor As Color

Public Event Changed(ByVal NewColor As Color)

I like that you have a single timer for the whole 60-second cycle, but the magic numbers in Lights() make it hard to correlate what you've done with what the requirements are. Using constants as above, helps making the code more readable. The fact that your logic relies on UI values isn't very nice, too.

When you have a TrafficLight that raises an event that says "Hey there, if it's of any interest to you, I'm changing my current color right now" to your form, the form itself doesn't know how the TrafficLight operates, but it knows that when color changes, it needs to change the background color of some PictureBox:

Private Sub NorthSouthLight_Changed(ByVal NewColor As Color)

    NorthTrafficLightPicture.BackColor = NewColor
    NorthLabel.BackColor = NewColor

    SouthTrafficLightPicture.BackColor = NewColor
    SouthLabel.BackColor = NewColor

End Sub

Private Sub EastWestLight_Changed(ByVal NewColor As Color)

    EastTrafficLightPicture.BackColor = NewColor
    EastLabel.BackColor = NewColor

    WestTrafficLightPicture.BackColor = NewColor
    WestLabel.BackColor = NewColor

End Sub

What color that is, isn't important - it's conveyed by the Changed event, if a TrafficLigth wanted to be Blue, the form would happily comply.


TL;DR: In other words, I think your code could greatly benefit from a little bit more of an Object-Oriented approach, especially if this code is (I was assuming all along, when I came across a Handles keyword, which I haven't seen used in VBA).

\$\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.