Shutdown Delay Program

Question

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 for Countdown / delay time values
 ElseIf Minutes = 0 And Hours = 0 Then
 MsgBox("Please enter a valid delay time", MsgBoxStyle.Exclamation, "Enter Delay")
 'Start Countdown Procedure
 Else
 Dim reply As MsgBoxResult 'Variable that stores reply value of message box
 reply = MsgBox("You are about to start the timer..." & vbNewLine & "Do You Want To Proceed?", MsgBoxStyle.YesNo, "Are You Sure?")
 If reply = MsgBoxResult.Yes Then
 tmrDelayCount.Enabled = True 'Enable Timer
 tbcTabs.Enabled = False 'Disable UI
 btnCancel.Enabled = True 'Enable Cancel Button
 'Display Confirm Message
 MsgBox("Your computer will shutdown in approximately: " & Hours & " Hours and " & Minutes & " Minutes " & vbNewLine & vbNewLine &
 "To stop the countdown, simply press the 'Cancel' button", MsgBoxStyle.OkOnly, "Countdown")
 Me.WindowState = FormWindowState.Minimized
 End If
 End If
 End Sub
 Private Sub btnSDTime_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnSDTime.Click
 Hours = CInt(cbxSetHour.Text)
 Minutes = CInt(cbxSetMin.Text)
 DelayOrTime = 2
 'Error check for Restart / Shutdown Radio buttons - make sure one is checked
 If radRestart_t.Checked = False And radShutdown_t.Checked = False Then
 MsgBox("Please select either 'Shutdown' or 'Restart'", MsgBoxStyle.Exclamation, "Select Option")
 'Start Countdown Procedure
 Else
 Dim reply As MsgBoxResult 'Variable that stores reply value of message box
 reply = MsgBox("You are about to start the procedure" & vbNewLine & "Do You Want To Proceed?", MsgBoxStyle.YesNo, "Are You Sure?")
 If reply = MsgBoxResult.Yes Then
 tmrCheckTime.Enabled = True 'Enable Timer
 'Disable UI and enable cancel
 tbcTabs.Enabled = False
 btnCancel.Enabled = True
 'Display Confirm Message
 MsgBox("Your computer will shutdown at approximately: " & Hours & " : " & Minutes & " " & vbNewLine & vbNewLine &
 "To stop the countdown, simply press the 'Cancel' button", MsgBoxStyle.OkOnly, "Countdown")
 Me.WindowState = FormWindowState.Minimized
 End If
 End If
 End Sub
 'Countdown Timer Subroutine
 Private Sub tmrDelayCount_Tick(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles tmrDelayCount.Tick
 'Display the countdown
 lblCountdown.Text = show_hrs & " : " & show_mins & " : " & show_secs 'Display Countdown
 'Decrement Seconds by 1
 Seconds -= 1
 'If the seconds is less than 0 reset to 59 and decrement the minutes
 If Seconds < 0 Then
 Seconds = 59
 Minutes -= 1
 End If
 'If the minutes is less than 0 reset to 59 and decrement the hours
 If Minutes < 0 Then
 Minutes = 59
 Hours -= 1
 End If
 show_hrs = CStr(Hours)
 show_mins = CStr(Minutes)
 show_secs = CStr(Seconds)
 'Display a '0' infront of the string when less than 10 eg '06'
 If Seconds < 10 Then
 show_secs = "0" + show_secs
 End If
 If Minutes < 10 Then
 show_mins = "0" + show_mins
 End If
 If Hours < 10 Then
 show_hrs = "0" + show_hrs
 End If
 'When Countdown reaches 00:00:00
 If Hours = 0 And Minutes = 0 And Seconds = 0 Then
 Call Shutdown_Restart()
 End If
 End Sub
 Private Sub tmrCheckTime_Tick(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles tmrCheckTime.Tick
 CurHour = Hour(Now)
 CurMinute = Minute(Now)
 If CurHour = Hours And CurMinute = Minutes Then
 Call Shutdown_Restart()
 End If
 End Sub
 Private Sub Shutdown_Restart()
 'If the user is shutting down on timer
 If DelayOrTime = 1 Then
 'Shutdown PC
 If radShutdown_d.Checked Then
 System.Diagnostics.Process.Start("Shutdown", "/s")
 tmrDelayCount.Enabled = False
 'Restart PC
 ElseIf radRestart_d.Checked Then
 System.Diagnostics.Process.Start("Shutdown", "/r")
 tmrDelayCount.Enabled = False
 End If
 'If the user is shutting down on certain time
 ElseIf DelayOrTime = 2 Then
 'Shutdown PC
 If radShutdown_t.Checked Then
 System.Diagnostics.Process.Start("Shutdown", "/s")
 tmrCheckTime.Enabled = False
 'Restart PC
 ElseIf radRestart_t.Checked Then
 System.Diagnostics.Process.Start("Shutdown", "/r")
 tmrCheckTime.Enabled = False
 End If
 End If
 End Sub
 Private Sub btnCancel_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnCancel.Click
 tmrCheckTime.Enabled = False
 tmrDelayCount.Enabled = False
 tbcTabs.Enabled = True
 btnCancel.Enabled = False
 If tmrCheckTime.Enabled = False And tmrDelayCount.Enabled = False Then
 MsgBox("Shutdown procedure was successfully cancelled", MsgBoxStyle.Information, "Shutdown Cancelled")
 Else
 MsgBox("Shutdown procedure was not cancelled!" & vbNewLine & "To stop the timer:" & vbNewLine &
 " - Open Task Manager" & vbNewLine &
 " - Under the 'Processes tab find the process called: WindowsApplication1.exe" & vbNewLine &
 " - Select the process and press the Delete key. CLick OK to the next dialog",
 MsgBoxStyle.Exclamation, "Cancellation Failed")
 End If
 End Sub
End Class

Here are also a couple of screenshots so you get the idea:

Screenshot1 Screenshot2

• Davidmh: I don't know this language, but if this were of any I know, it would benefit from splitting it into logically coherent modules.
• Stormcloud: I know the question is really asking for a code review, but I can't help thinking that there are already solutions to the problem of shutting down a much at a particular time - so no programming is required. Personally I'd just install Cygwin (or something similar) and use the shutdown command.
• the_lotus: If you want to shutdown in 3 hours, just store the current date/time + 3hour. And then just compare if DateTime.Now > DateTimeToShutDown. You don't need to store the minute/second/... and then you don't need to decrement each second.

Answer 1

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

• 200_success: On the other hand, DelayOrTime needs a better comment than that.
• RubberDuck: @200_success DelayOrTime probably shouldn't exist, but I've not had time to sit down with my IDE yet.
• Heslacher: ` For Each cntrl in frmMain.Controls: here you assume that all the comboboxes are inside the forms control collection. Also a cast is needed to access the SelectedIndex` property.
• Mathieu Guindon: What happens if radShutdown_d is checked?
• RubberDuck: Yeah @ImClarky, you'll find very few programmers outside of academia that like that approach. Comments should tend to say "Why" instead of "What" (unless that "what" is really really obscure).

Answer 2

I recently tinkered with it to allow me to have my PC shutdown at a specific time.

That's what happens to code: it needs to change, at one point or another. The one major problem you have here, is that everything is stuffed in the form's code-behind.

From a design-pattern point of view, what you have is a Smart UI - the View is stealing the show, and contains the entire program.

This kind of approach is not always bad. It is fine if you want something quick and simple that isn’t going to change. For example if you want to create a quick demo, or explore how something works.

However, most of the time an application will change and that’s where things will get messy, particular if you have a lot of Smart UIs in one application.

Maintenance will get difficult. A supposedly simple change could affect anything else associated with the same class. Over time the code will likely messier and messier as more maintainers work with the same code.

_Link

What you've called frmMain is a class, a first-class citizen of the language you're using. Classes should only have a single purpose. If we agree that a Smart UI isn't what we're looking for, then you need more classes - think of what the responsibilities of each class should be.

---

###Model

Your Model is your data - in this case, it's bits & pieces of information, like:

• Some time span that indicates the delay between now and shutdown/restart.
• Some bit that indicates whether we want to shutdown or restart.
• Some date/time that indicates exactly when we're going to shutdown/restart.

You can write a class that's responsible for containing that information (here with a placeholder name):

Public Enum OnTimerElapsedAction
 Shutdown
 Restart
End Enum
Public Class Model
 Public Property SelectedTimeSpan As TimeSpan
 Public Property SelectedDateTime As DateTime
 Public Property SelectedAction As OnTimerElapsedAction
End Class

###View

The form is your View, and should be nothing more than that: the application's logic shouldn't be written in the form's code-behind, burried in button click event handler procedures. I suggest you look into the Model-View-Presenter pattern.

Redundancies

The Shutdown_Restart() method clearly is doing way too many things. In fact, it's such a central part of your application, that it belongs in its very own, specialized class, exposing two methods:

Public Sub Shutdown()
Public Sub Restart()

What's the difference between this:

 If DelayOrTime = 1 Then
 'Shutdown PC
 If radShutdown_d.Checked Then
 System.Diagnostics.Process.Start("Shutdown", "/s")
 tmrDelayCount.Enabled = False
 'Restart PC
 ElseIf radRestart_d.Checked Then
 System.Diagnostics.Process.Start("Shutdown", "/r")
 tmrDelayCount.Enabled = False
 End If

And this:

 ElseIf DelayOrTime = 2 Then
 'Shutdown PC
 If radShutdown_t.Checked Then
 System.Diagnostics.Process.Start("Shutdown", "/s")
 tmrCheckTime.Enabled = False
 'Restart PC
 ElseIf radRestart_t.Checked Then
 System.Diagnostics.Process.Start("Shutdown", "/r")
 tmrCheckTime.Enabled = False
 End If
 End If

?

You need a method that parameterizes the variables here, and write code once - you're repeating yourself.

---

I'd like to call out on this line:

If CurHour = Hours And CurMinute = Minutes Then
 Call Shutdown_Restart()
End If

I don't understand why you're using Call here. Please see MSDN for appropriate usage.

---

###Strings

You don't need to have this:

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

And looking here...

'Display the countdown
lblCountdown.Text = show_hrs & " : " & show_mins & " : " & show_secs 'Display Countdown

Two things: one has already been mentioned by @RubberDuck - the comments are maddening, remove them. The other thing is that by concatenating String objects like this, you're actually creating many more String objects than you think you're creating.

A String in .net is an immutable type. This means you're creating a whole bunch of objects. Let's see:

lblCountdown.Text = show_hrs & " : " & show_mins & " : " & show_secs

• lblCountdown.Text is one.
• show_hrs is another.
• " : " is a third.
• show_mins is a fourth.
• " : " would be yet another, but the compiler is smart enough to use the same instance as the 3rd (since .net 2.0 that is).
• show_secs makes it six.
• show_hrs & " : " is a seventh.
• show_hrs & " : " & show_mins and we're at eight.
• show_hrs & " : " & show_mins & " : " is nine.
• show_hrs & " : " & show_mins & " : " & show_secs was already counted, it's the reference we're assigning to lblCountdown.Text

So 8 objects created, when all you really need, is to format a time span into a string. TimeSpan.ToString() can do that. vb.net isn't just a language - ".NET" is a framework, getting to know what it's made of can only be beneficial; I highly recommend you take a look at what the System namespace has in store for you:

• System.String
• System.DateTime
• System.TimeSpan

---

Private Sub frmMain_Load(ByVal sender As System.Object, ByVal e As System.EventArgs)

Handles MyBase.Load

If you're importing the System namespace in your code file, there is no reason to fully qualify System.Object and System.EventArgs - you can dismiss the namespace and simply refer to the type names, Object and EventArgs. Doing so removes clutter.

---

There is a lot more that can be said about this code. In my opinion the most pressing concern is the Smart UI. When you're done refactoring, the form should only be concerned with... presentation concerns; the actual logic belongs elsewhere.

• ImClarky: Thanks for this detailed response. I think I'm going to need some tea and biscuits to help me digest what you have written:) As mentioned in the comment I put on RubberDuck's answer, I only know what I was taught at school, which was pretty basic as it was mainly an introduction into programming. But I will read you answer in depth when the kettle has boiled, and make the adjustments as I understand them.
• RubberDuck: @ImClarky I really recommend a follow up question once you have your refactored version together. There was a lot of information to absorb. I'm sure you'll want to know if you actually absorbed it and did well with your rewrite.
• Mathieu Guindon: ...and I didn't include half of what I wanted to...
• ImClarky: @Mat'sMug ..... Is it really that bad :(
• RubberDuck: @ImClarky, well, it works and I'm fond of the adage that "We ship features, not code."

Answer 3

I know you asked for Code Review but this feature has been built into Windows since Win98 (Nothing to install).

shutdown [{-l|-s|-r|-a}] [-f] [-m [\\ComputerName]] [-t xx] [-c "message"] [-d[u][p]:xx:yy] 

Parameters

-l : Logs off the current user, this is also the defualt. -m ComputerName takes precedence. 
-s : Shuts down the local computer. 
-r : Reboots after shutdown. 
-a : Aborts shutdown. Ignores other parameters, except -l and ComputerName. You can only use -a during the time-out period. 
-f : Forces running applications to close. 
-m [\\ComputerName] : Specifies the computer that you want to shut down. 
-t xx : Sets the timer for system shutdown in xx seconds. The default is 20 seconds. 
-c "message" : Specifies a message to be displayed in the Message area of the System Shutdown window. You can use a maximum of 127 characters. You must enclose the message in quotation marks. 
-d [u][p]:xx:yy : Lists the reason code for the shutdown. The following table lists the different values.

If you want to shutdown your computer after 3 hours, you could do this from a command prompt or your RUN window.

shutdown /s /t 10800

REFERENCE: Technet