# Shutdown Delay Program

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
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?")
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
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?")
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
System.Diagnostics.Process.Start("Shutdown", "/s")
tmrDelayCount.Enabled = False

'Restart PC
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
System.Diagnostics.Process.Start("Shutdown", "/s")
tmrCheckTime.Enabled = False

'Restart PC
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:

-
I don't know this language, but if this were of any I know, it would benefit from splitting it into logically coherent modules. –  Davidmh Sep 2 at 1:14
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. –  Stormcloud Sep 2 at 15:13
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. –  the_lotus Sep 5 at 18:46

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

System.Diagnostics.Process.Start("Shutdown","/s")
System.Diagnostics.Process.Start("Shutdown","/r")
End If
End Sub

-
On the other hand, DelayOrTime needs a better comment than that. –  200_success Sep 2 at 1:10
@200_success DelayOrTime probably shouldn't exist, but I've not had time to sit down with my IDE yet. –  RubberDuck Sep 2 at 1:41
 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. –  Heslacher Sep 2 at 6:26
What happens if radShutdown_d is checked? –  Mat's Mug Sep 2 at 13:48
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). –  RubberDuck Sep 2 at 14:56

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

http://geekswithblogs.net/gregorymalcolm/archive/2009/07/14/user-interface-patterns.aspx

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
System.Diagnostics.Process.Start("Shutdown", "/s")
tmrDelayCount.Enabled = False

'Restart PC
System.Diagnostics.Process.Start("Shutdown", "/r")
tmrDelayCount.Enabled = False
End If


And this:

    ElseIf DelayOrTime = 2 Then
'Shutdown PC
System.Diagnostics.Process.Start("Shutdown", "/s")
tmrCheckTime.Enabled = False

'Restart PC
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. 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:

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


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.

-
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. –  ImClarky Sep 2 at 14:56
@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. –  RubberDuck Sep 2 at 17:01
...and I didn't include half of what I wanted to... –  Mat's Mug Sep 2 at 17:44
@Mat'sMug ..... Is it really that bad :( –  ImClarky Sep 2 at 19:35
@ImClarky, well, it works and I'm fond of the adage that "We ship features, not code." –  RubberDuck Sep 2 at 19:40

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

-