Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

share|improve this question
3  
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

3 Answers 3

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
share|improve this answer
3  
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
2  
` 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
1  
What happens if radShutdown_d is checked? –  Mat's Mug Sep 2 at 13:48
1  
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
        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. 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.

share|improve this answer
2  
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
1  
@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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.