Power loss notifier

Question

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;
            }
        }
    }
}

Answer 1

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

Answer 2

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.