HiveBrain v1.2.0
Get Started
← Back to all entries
patternModerate

Shutdown Delay Program

Submitted by: @import:stackexchange-codereview··
0
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

Solution

I can't resist mentioning this. These comments hurt me.

'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


It'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
Next


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


Mat'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 Sub

Code 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 time
For Each cntrl in frmMain.Controls
    If TypeOf cntrl Is ComboBox Then
        cntrl.SelectedIndex = 0
    End If
Next
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.Delayed
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 Sub

Context

StackExchange Code Review Q#61726, answer score: 11

Revisions (0)

No revisions yet.