# Traffic light implementation

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
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


### 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).