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

En

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 vb.net (I was assuming vba all along, when I came across a Handles keyword, which I haven't seen used in VBA).

StackExchange Code Review Q#48943, answer score: 5