6
\$\begingroup\$

My laptop is old, so I decided it would be good if my laptop would scream at me if I kick power brick, so I would make the brick work again, and not notice only when the battery runs out and I lose all the data (battery meter is not to be trusted).

namespace POWER {
    class Program {
        private static System.Speech.Synthesis.SpeechSynthesizer synthesizer;
        private static System.Windows.Forms.NotifyIcon theIcon;
        [System.STAThread]
        static void Main(string[] args) {
            synthesizer = new System.Speech.Synthesis.SpeechSynthesizer();
            synthesizer.SetOutputToDefaultAudioDevice();
            theIcon = new System.Windows.Forms.NotifyIcon();
            theIcon.Icon = System.Drawing.SystemIcons.Asterisk;
            theIcon.ContextMenu = new System.Windows.Forms.ContextMenu();
            theIcon.ContextMenu.MenuItems.Add("Exit", (s, e) => {
                theIcon.Visible = false;
                System.Windows.Forms.Application.Exit();
            });
            theIcon.MouseClick += (s, e) => {
                if(e.Button == System.Windows.Forms.MouseButtons.Left) {
                    //make both mouse buttons show context menu
                    var method = typeof(System.Windows.Forms.NotifyIcon).GetMethod("ShowContextMenu", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic);
                    method.Invoke(theIcon, null);
                }
            };
            var timer=new System.Windows.Forms.Timer();
            timer.Interval = 3000;
            timer.Tick += (s, e) => {
                Tick();
            };
            timer.Enabled = true;
            Tick();
            theIcon.ContextMenu.MenuItems.Add(0,new System.Windows.Forms.MenuItem("Status",(s,e)=>{
                var status=System.Windows.Forms.SystemInformation.PowerStatus;
                synthesizer.SpeakAsync(string.Format("power line {0}, battery {1} {2}%", status.PowerLineStatus, status.BatteryChargeStatus, status.BatteryLifePercent * 100));
            }));
            synthesizer.SpeakAsync("almost ready.");

            theIcon.Visible = true;
            System.Windows.Forms.Application.Run();
        }

        private static void Tick() {
            var status=System.Windows.Forms.SystemInformation.PowerStatus;
            if(status.PowerLineStatus == System.Windows.Forms.PowerLineStatus.Offline) {
                synthesizer.SpeakAsync("power line disconnected!");
                theIcon.Icon = System.Drawing.SystemIcons.Error;
            } else {
                theIcon.Icon = System.Drawing.SystemIcons.Application;
            }
        }
    }
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ If the battery meter is not to be trusted, then what makes you think this app will be any different? You're tapping into the same system-level API. \$\endgroup\$
    – Cody Gray
    Oct 9, 2017 at 13:04
  • \$\begingroup\$ @CodyGray Well supposing the idea is that OP only wants notice when they lose power connection this should suffice. I have a laptop with such a bad battery that when AC power is lost I get ~2.7 seconds before it shuts down, so even this is (sadly) not usable for it, but if the OP gets 30-60 seconds of life when kicking to AC power, then this should work for that scenario. \$\endgroup\$ Oct 9, 2017 at 13:26

2 Answers 2

7
\$\begingroup\$

First of all feel free to add some using ... here and there, it will make your code easier to read.

You may want to create separate classes to group responsibilities but code is so short that it's just over-engineering. I'd, however, first move initialization into a separate method, I'd want to have a Main() like this:

void Main()
{
    InitializeSpeechSynthesizer();
    InitializeSystemTrayIcon();

    CheckPowerStatus();

    Application.Run();
}

When reading the application entry point you have a quick overview of what it is doing and of where you have to go to change anything (not a problem now but it will otherwise waste your valuable time after 6 months when you will want to add a new small feature...)

I leave out InitializeSpeechSynthesizer() but you should add some exception handling there, at least to quit your application with a nice error message instead of a crash.

In InitializeSystemTrayIcon() first of all rename theIcon to something more meaningful, trayIcon.

trayIcon = new NotifyIcon();
trayIcon.Icon = SystemIcons.Asterisk;
trayIcon.ContextMenu = new ContextMenu();
trayIcon.ContextMenu.MenuItems.Add("Exit", OnApplicationExit);

Here, besides the namespace thing, I introduced a separate method to handle the click event on the menu. Do not pollute your initialization code with behaviors, it will quickly become an unreadable mess. They may even be local functions (if you're using C# 7).

static void OnApplicationExit(object sender, EventArgs e)
    => Application.Exit();

The same is true for MouseClick event:

trayIcon.MouseClick += OnIconClicked;

With:

static void OnIconClicked(object sender, MouseEventArgs e)
{
    if (e.Button == MouseButtons.Left)
        ShowTrayIconContextMenu();
}

Again the point here is to immediately understand what happens in response to an event, without reading the effective implementation.

Let's go back to context menu initialization, why do you interleave so many other things? You shouldn't need to read the whole code to see everything related to one specific object:

trayIcon = new NotifyIcon();
trayIcon.Icon = SystemIcons.Asterisk;
trayIcon.Visible = true;
trayIcon.MouseClick += OnIconClicked;
trayIcon.ContextMenu = new ContextMenu();
trayIcon.ContextMenu.MenuItems.Add("Exit", OnApplicationExit);
trayIcon.ContextMenu.MenuItems.Add("Status", OnSpeechStatus);

With:

static void OnSpeechStatus(object sender, EventArgs e)
{
    var status = SystemInformation.PowerStatus;
    synthesizer.SpeakAsync(string.Format("power line {0}, probably battery is {1} {2}%",
        status.PowerLineStatus,
        status.BatteryChargeStatus,
        (int)(status.BatteryLifePercent * 100)));
}

Note truncation of status.BatteryLifePercent * 100 to an integer number, no one is interested to know that battery is charged at 67.4635%.

What I said (avoid anonymous delegates inside initialization code) applies also to the timer initialization.

var timer = new Timer();
timer.Interval = 3000;
timer.Tick += OnCheckPowerStatusTimer;
timer.Enabled = true;

With:

static void OnCheckPowerStatusTimer(object sender, EventArgs e)
    => CheckPowerStatus();

And:

static void CheckPowerStatus()
{
    if (SystemInformation.PowerStatus.PowerLineStatus == PowerLineStatus.Offline)
    {
        synthesizer.SpeakAsync("power line disconnected!");
        trayIcon.Icon = SystemIcons.Error;
    }
    else
    {
        trayIcon.Icon = SystemIcons.Application;
    }
}

You may want to move that hard-coded 3000 to a private const int PowerStatusPollingTime = 3000 constant. If you will decide to change it you won't need to find it inside your code (especially if it will happen to re-use it in multiple places).

Discussions and open points

Do you really want to hear almost done every single time the application starts? It's your own program then you're free to choose but...

You may consider to keep right-click to open menu and left click to speak out battery status.

Do you really want to hear power line disconnected every 3 seconds when it happens? You will quickly crash your computer or close the application! You may: change the timer to do this check every minute (after you detected that change) or - better - simply mute the voice after a fixed amount of repetitions. You may also perform this check less often than 3 seconds, BTW. Few more UX things but I guess they're pretty off-topic for a CR answer...

\$\endgroup\$
4
  • \$\begingroup\$ I actually want laptop to yell at me continiously, and notice power loss quickly, thus 3000ms, to make speech not overlap, but close \$\endgroup\$ Oct 9, 2017 at 9:23
  • 3
    \$\begingroup\$ It sounds like a "nuclear meltdown, nuclear meltdown, nuclear meltdown..." warning! ;) \$\endgroup\$ Oct 9, 2017 at 9:33
  • \$\begingroup\$ I guess OP doesn't have a minute which would be understandable if the battery is old and cannot store much power anymore ;-) \$\endgroup\$
    – t3chb0t
    Oct 9, 2017 at 11:24
  • 2
    \$\begingroup\$ Then let's change it to sci-fi movie style! "System is about to shutdown in 1 minute, system is about to shutdown in 45 seconds, system..." \$\endgroup\$ Oct 9, 2017 at 13:04
6
\$\begingroup\$

Rename to useful variables

  • theIcon? The word “the” adds nothing to the variable meaning.
  • var "method"? Anything apart from calling it "method"!!!

Order and simplify the code

  • Code pertaining to the icon are needlessly interspersed throughout the code. I’ve restructured it a little bit.
  • Break it down into smaller methods. Makes it easier to read and understand. You’ll read your code many more times than you edit it, so it’s vital to make it readable.
  • Personally I like using regions, but it’s debatable.

Make use of the using directive

  • no need to write the full namespace. System.Windows.Forms etc. etc. just make use of the using directives. I’ve added those statements - notice how much easier it reads. See below.

    internal class Program //// Warning: Untested code
    {
    private static SpeechSynthesizer synthesizer;
    private static NotifyIcon icon;
    [System.STAThread]
    private static void Main(string[] args)
    {
        SetupSynthesizer();
    
        SetupIcon();
    
        SetupTimer();
    
        synthesizer.SpeakAsync("Almost ready.");
    
        Application.Run();
    }
    
    #region Icon methods
    
    private static void SetupIcon()
    {
        icon = new NotifyIcon();
        icon.Icon = SystemIcons.Asterisk;
    
        AddIconMenu();
    
        AddIconClickHandler();
    
        icon.Visible = true;
    }
    
    private static void AddIconClickHandler()
    {
        icon.MouseClick += (s, e) =>
        {
            if (e.Button == MouseButtons.Left)
            {
                ShowContext();
            }
        };
    }
    
    private static void ShowContext()
    {
        //make both mouse buttons show context menu
        var showContext = typeof(NotifyIcon).GetMethod("ShowContextMenu", BindingFlags.Instance | BindingFlags.NonPublic);
        showContext.Invoke(icon, null);
    }
    
    private static void AddIconMenu()
    {
        icon.ContextMenu = new ContextMenu();
    
        icon.ContextMenu.MenuItems.Add("Exit", (s, e) =>
        {
            icon.Visible = false;
            Application.Exit();
        });
    
        icon.ContextMenu.MenuItems.Add(0, new MenuItem("Status", (s, e) =>
        {
            var status = SystemInformation.PowerStatus;
            synthesizer.SpeakAsync(string.Format("power line {0}, battery {1} {2}%", status.PowerLineStatus, status.BatteryChargeStatus, status.BatteryLifePercent * 100));
        }));
    }
    
    #endregion Icon methods
    
    #region Timer
    
    private static void SetupTimer()
    {
        var timer = new Timer();
        timer.Interval = 3000;
        timer.Tick += (s, e) =>
        {
            Tick();
        };
    
        timer.Enabled = true;
        Tick();
    }
    
    private static void Tick()
    {
        var status = SystemInformation.PowerStatus;
        if (status.PowerLineStatus == PowerLineStatus.Offline)
        {
            synthesizer.SpeakAsync("power line disconnected!");
            icon.Icon = SystemIcons.Error;
        }
        else
        {
            icon.Icon = SystemIcons.Application;
        }
    }
    
    #endregion Timer
    
    #region Synthesizer
    
    private static void SetupSynthesizer()
    {
        synthesizer = new SpeechSynthesizer();
        synthesizer.SetOutputToDefaultAudioDevice();
    }
    
    #endregion Synthesizer
    }
    

And the using statements:

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading.Tasks;
using System.Drawing;
using System.Windows.Forms;
using System.Speech.Synthesis;
using System.Reflection;

Handlers

@AdrianoRepetti makes some interesting comments re: error handling and click event handlers that are well worth considering.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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