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

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, yesterday
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, 20 hours ago

RubberDuck · Answer 1 · 2014-09-02 13:52:37Z

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

On the other hand, DelayOrTime needs a better comment than that. — 200_success♦, yesterday
@200_success DelayOrTime probably shouldn't exist, but I've not had time to sit down with my IDE yet. — RubberDuck, yesterday
+1 for the code comments. @ImClarky you should probably aim on removing most of your comments and write the code in such a way that it is clear what you are attempting to do. — SebastianSG, yesterday
` 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, yesterday

Mat's Mug · Answer 2 · 2014-09-02 04:41:53Z

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.

_{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
        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.
`" : " is yet another.
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 9 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:

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.

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, 21 hours ago
@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, 19 hours ago
@ImClarky, well, it works and I'm fond of the adage that "We ship features, not code." — RubberDuck, 16 hours ago

asked	yesterday
viewed	371 times
active	today

current community

your communities

more stack exchange communities

Shutdown Delay Program

2 Answers 2

Model

View

Strings

Your Answer

Not the answer you're looking for? Browse other questions tagged beginner datetime vb.net winforms or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Shutdown Delay Program

2 Answers 2

Model

View

Strings

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged beginner datetime vb.net winforms or ask your own question.

Related

Hot Network Questions