patternModerate
Shutdown Delay Program
Viewed 0 times
shutdownprogramdelay
Problem
I wrote this program a while back that would allow me to set a delay, say 3 hours, and when the delay had expired, my PC would turn off. The purpose of it was so I didn't have to leave my PC on all night whilst downloading games. I recently tinkered with it to allow me to have my PC shutdown at a specific time. It works like a treat, but I was hoping on a few pointers to help make the code more concise / efficient / cleaner.
`Public Class frmMain
'Declaration of Variables
Public Seconds As Integer 'Store Seconds as Integer
Public Minutes As Integer 'Store Minutes as Integer
Public Hours As Integer 'Store Hours as Integer
Public CurHour As Integer 'Store the Current Hour as Integer
Public CurMinute As Integer 'Store the Current Minute as Integer
Public DelayOrTime As Integer 'Varibale used to distinguish between Shutdown on Delay or Shutdown on certain time
'Used for displaying delay time
Public show_hrs As String 'Store hours as String
Public show_mins As String 'Store minutes as String
Public show_secs As String 'Store seconds as String
Private Sub frmMain_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
'Reset dropdowns
cbxHours.SelectedIndex = 0
cbxMinutes.SelectedIndex = 0
cbxSetHour.SelectedIndex = 0
cbxSetMin.SelectedIndex = 0
DelayOrTime = 0
End Sub
Private Sub btnDelay_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnSDDelay.Click
'Convert Dropdown values to Integers
Minutes = CInt(cbxMinutes.Text)
Hours = CInt(cbxHours.Text)
Seconds = 0
DelayOrTime = 1
'Error check for Restart / Shutdown Radio buttons - make sure one is checked
If radRestart_d.Checked = False And radShutdown_d.Checked = False Then
MsgBox("Please select either 'Shutdown' or 'Restart'", MsgBoxStyle.Exclamation, "Select Option")
'Error check
`Public Class frmMain
'Declaration of Variables
Public Seconds As Integer 'Store Seconds as Integer
Public Minutes As Integer 'Store Minutes as Integer
Public Hours As Integer 'Store Hours as Integer
Public CurHour As Integer 'Store the Current Hour as Integer
Public CurMinute As Integer 'Store the Current Minute as Integer
Public DelayOrTime As Integer 'Varibale used to distinguish between Shutdown on Delay or Shutdown on certain time
'Used for displaying delay time
Public show_hrs As String 'Store hours as String
Public show_mins As String 'Store minutes as String
Public show_secs As String 'Store seconds as String
Private Sub frmMain_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
'Reset dropdowns
cbxHours.SelectedIndex = 0
cbxMinutes.SelectedIndex = 0
cbxSetHour.SelectedIndex = 0
cbxSetMin.SelectedIndex = 0
DelayOrTime = 0
End Sub
Private Sub btnDelay_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnSDDelay.Click
'Convert Dropdown values to Integers
Minutes = CInt(cbxMinutes.Text)
Hours = CInt(cbxHours.Text)
Seconds = 0
DelayOrTime = 1
'Error check for Restart / Shutdown Radio buttons - make sure one is checked
If radRestart_d.Checked = False And radShutdown_d.Checked = False Then
MsgBox("Please select either 'Shutdown' or 'Restart'", MsgBoxStyle.Exclamation, "Select Option")
'Error check
Solution
I can't resist mentioning this. These comments hurt me.
It's pretty obvious that you're storing seconds as an integer. The comments just clutter the code and make it noisy.
In
It's more lines of code, but completely maintenance free if you should add another ComboBox.
I just noticed that all of the class variables are
As I briefly mentioned in the comments,
Mat's Mug mentioned your
'Declaration of Variables
Public Seconds As Integer 'Store Seconds as Integer
Public Minutes As Integer 'Store Minutes as Integer
Public Hours As Integer 'Store Hours as Integer
Public CurHour As Integer 'Store the Current Hour as Integer
Public CurMinute As Integer 'Store the Current Minute as Integer
Public DelayOrTime As Integer 'Varibale used to distinguish between Shutdown on Delay or Shutdown on certain timeIt's pretty obvious that you're storing seconds as an integer. The comments just clutter the code and make it noisy.
In
frmMain_Load there's a lot of repetition. I would use a loop here instead. For Each cntrl in frmMain.Controls
If TypeOf cntrl Is ComboBox Then
cntrl.SelectedIndex = 0
End If
NextIt's more lines of code, but completely maintenance free if you should add another ComboBox.
I just noticed that all of the class variables are
Public. Why? These should probably all be private. I can't think of a reason to expose them outside of the class. As I briefly mentioned in the comments,
DelayOrTime is a bit clunky. It has one of two possible values, 1 or 2. Magic numbers are bad. Avoid them. Normally, if there are only two possible states, I would recommend a boolean, but that doesn't feel quite right to me here. I would create an Enum and change the variable name to Mode. Private Enum ExecMode
Delayed
Timed
End Enum
Private Sub btnDelay_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnSDDelay.Click
'Convert Dropdown values to Integers
Minutes = CInt(cbxMinutes.Text)
Hours = CInt(cbxHours.Text)
Seconds = 0
Mode = ExecMode.DelayedMat's Mug mentioned your
Shutdown_Restart method. I don't like it either, but I'm not sure that I agree with his suggestion. I think it could be cleaned up by simply grouping the logic together correctly.Private Sub Shutdown_Restart()
If DelayOrTime = 1 Then
tmrDelayCount.Enabled = False
ElseIf DelayOrTime = 2 Then
tmrCheckTime.Enabled = False
End If
If radShutdown_t.Checked OrElse radShutdown_d.Checked Then
System.Diagnostics.Process.Start("Shutdown","/s")
ElseIf radRestart_d.Checked OrElse radShutdown_d.Checked Then
System.Diagnostics.Process.Start("Shutdown","/r")
End If
End SubCode Snippets
'Declaration of Variables
Public Seconds As Integer 'Store Seconds as Integer
Public Minutes As Integer 'Store Minutes as Integer
Public Hours As Integer 'Store Hours as Integer
Public CurHour As Integer 'Store the Current Hour as Integer
Public CurMinute As Integer 'Store the Current Minute as Integer
Public DelayOrTime As Integer 'Varibale used to distinguish between Shutdown on Delay or Shutdown on certain timeFor Each cntrl in frmMain.Controls
If TypeOf cntrl Is ComboBox Then
cntrl.SelectedIndex = 0
End If
NextPrivate Enum ExecMode
Delayed
Timed
End Enum
Private Sub btnDelay_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnSDDelay.Click
'Convert Dropdown values to Integers
Minutes = CInt(cbxMinutes.Text)
Hours = CInt(cbxHours.Text)
Seconds = 0
Mode = ExecMode.DelayedPrivate Sub Shutdown_Restart()
If DelayOrTime = 1 Then
tmrDelayCount.Enabled = False
ElseIf DelayOrTime = 2 Then
tmrCheckTime.Enabled = False
End If
If radShutdown_t.Checked OrElse radShutdown_d.Checked Then
System.Diagnostics.Process.Start("Shutdown","/s")
ElseIf radRestart_d.Checked OrElse radShutdown_d.Checked Then
System.Diagnostics.Process.Start("Shutdown","/r")
End If
End SubContext
StackExchange Code Review Q#61726, answer score: 11
Revisions (0)
No revisions yet.