19
\$\begingroup\$

In my previous question I got some pretty good suggestions which really improved the overall performance of the controls, but I decided to extend the application and add some more features :

  1. You can enable/disable auto-updating.
  2. You can select which processes you want to view currently available options are :

    • Default - shows all the processes.

    • Active - shows only active processes.

    • Foreground - show only foreground processes.

  3. Upon minimizing the application will move to the tray bar (there is small bug there sometimes 2 icons will show, but the second will fade after few seconds.)

  4. You can inspect processes, which shows 3 more additional properties :

    • Instances running - the amount of active instances of the process.

    • Process Path - the directory where the .exe is located.

    • Background Process - Indicates whether a process is background process or not (true/false).

    • Owner - the user that runs the process DOMAIN / user.

  5. You can also terminate and refresh processes.

  6. Searching options is available with auto suggest from the current shown list of processes.

Note

The application now requires administrator privileges as most processes are hidden if not run as administrator + it prevents most of the exceptions, tho there are few processes that will still deny access.

That pretty much sums up all the new things and this is how the program looks like now :

enter image description here

Let's start inspecting the code than.

BufferedListView

As suggested in the previous question's answer I'm not using a simple class that "extendeds" the ListView control by setting the DoubleBuffered property to true by default, to reduce flickering.

public sealed class BufferedListView : ListView
{
    public BufferedListView()
    {
        DoubleBuffered = true;
    }

    protected override bool DoubleBuffered { get; set; }
}

Extensions.cs

The new extension methods :

  1. RemoveAll(this IDictionary<,>) with 2 overloads, it works the same way as a List.RemoveAll() would, but for some reason dictionaries don't have that built-in.

  2. SplitOnCapitalLetters(this string) splits a string on each capital letter.

Class implementation :

public static class Extensions
{
    public static string ExtendWithEmptySpaces(this string mainString, int desiredLength)
    {
        if (mainString.Length >= desiredLength)
        {
            return mainString;
        }
        StringBuilder extendedStringBuilder = new StringBuilder(mainString);
        for (int i = 0; i < desiredLength - mainString.Length; i++)
        {
            extendedStringBuilder.Append(" ");
        }
        return extendedStringBuilder.ToString();
    }

    public static int GetAverageLengthOf<T>(this IEnumerable<T> collection, Func<T, int> predicate)
        => (int) Math.Ceiling(collection.Average(predicate.Invoke));

    public static void RemoveAll<K, V>(this IDictionary<K, V> dict, Func<K, V, bool> match)
    {
        foreach (var key in dict.Keys.ToArray().Where(key => match(key, dict[key])))
        {
            dict.Remove(key);
        }
    }
    //this one is slower but prettier.
    public static void RemoveAll<K, V>(this IDictionary<K, V> dict, Func<KeyValuePair<K, V>, bool> match)
    {
        foreach (var key in dict.Keys.ToArray().Where(key => match.Invoke(new KeyValuePair<K, V>(key, dict[key]))))
        {
            dict.Remove(key);
        }
    }

    public static string SplitOnCapitalLetters(this string inputString)
    {
        StringBuilder result = new StringBuilder();
        foreach (var ch in inputString)
        {
            if (char.IsUpper(ch) && result.Length > 0)
            {
                result.Append(' ');
            }
            result.Append(ch);
        }
        return result.ToString();
    }
}

ProcessInfo.cs

The small wrapper class ProcessInfo is now moved to a separate file. The content remains unchanged :

internal class ProcessInfo
{
    public Process Process { get; }
    public TimeSpan TimeActive { get; set; }

    public ProcessInfo(Process process, TimeSpan timeActive)
    {
        Process = process;
        TimeActive = timeActive;
    }
}

ProcessInspector.cs

I've moved all of the general purpose methods operating on objects of type Process to a separate dedicated class called ProcessInspector.

public static class ProcessInspector
{
    public static string GetProcessPath(Process process)
    {
        try
        {
            string query = "SELECT ExecutablePath FROM Win32_Process WHERE ProcessId = " + process.Id;
            var searcher = new ManagementObjectSearcher(query);
            var collection = searcher.Get();
            return collection.Cast<ManagementObject>().First()["ExecutablePath"].ToString();
        }
        catch
        {
            return string.Empty;
        }
    }

    public static string GetProcessOwner(int processId)
    {
        string query = "SELECT * FROM Win32_Process WHERE ProcessId = " + processId;
        var searcher = new ManagementObjectSearcher(query);
        var processList = searcher.Get();
        var managementObject = processList.Cast<ManagementObject>().First();
        string[] argList = { string.Empty, string.Empty };
        return Convert.ToInt32(managementObject.InvokeMethod("GetOwner", argList)) == 0
            ? argList[1] + @"\" + argList[0]
            : "NO OWNER";
    }

    public static int GetActiveProcessId()
    {
        IntPtr activatedHandle = GetForegroundWindow();
        if (activatedHandle == IntPtr.Zero)
        {
            return -1;
        }
        int activeProcessId;
        GetWindowThreadProcessId(activatedHandle, out activeProcessId);
        return activeProcessId;
    }

    public static bool IsBackgroundProcess(Process p)
    {
        IntPtr hnd = p.MainWindowHandle;
        const uint WS_DISABLED = 0x8000000;
        const int GWL_STYLE = -0x10;
        bool visible = false;
        if (hnd != IntPtr.Zero)
        {
            int style = GetWindowLong(hnd, GWL_STYLE);
            visible = (style & WS_DISABLED) != WS_DISABLED;
        }
        return !visible;
    }

    [DllImport("user32.dll", CharSet = CharSet.Auto, ExactSpelling = true)]
    private static extern IntPtr GetForegroundWindow();

    [DllImport("user32.dll", CharSet = CharSet.Auto, SetLastError = true)]
    private static extern int GetWindowThreadProcessId(IntPtr handle, out int processId);

    [DllImport("user32.dll")]
    private static extern int GetWindowLong(IntPtr hWnd, int nIndex);
}

Moving on to the last and longest part of the code, the actual form. Now here, we have quite some lines of code (452) and since I don't see a good reason to refactor the class into smaller ones (if you do please point that out in an answer), in order to reduce the size of the file and make it easier to read I've separated the variables from the actual logic in 2 files public partial class MainFormVariables where all the variables are stored and public partial class MainForm : Form where everything else is stored.

I've also added a #region for all of the event handlers and turned all of the expression bodied members to return statements so that they can collapse too.

But before I show you the code, I'd like to point out that, thanks to the good suggestions in the previous question's answers the actual update of the controls takes almost no time, but the bottleneck here appears to be the functions that retrieve the process' properties values, which I just can't think of a way to improve as I can't store them since they are constantly changing (right ?) and they are pretty much one liners.. Enough talking here's the code :

MainFormVariables.cs

public partial class MainForm
{
    private enum ProcessProperties
    {
        Id,
        Name,
        InstancesRunning,
        Status,
        Owner,
        TotalRuntime,
        ActiveRuntime,
        StartTime,
        MemoryUsage,
        ProcessPath,
        BackgroundProcess,
    }

    private enum OrderBy
    {
        Ascending,
        Descending,
    }

    private enum ShowOptions
    {
        Default,
        Active,
        Foreground,
    }

    private static readonly HashSet<Process> blackList = new HashSet<Process>();

    private static readonly Dictionary<int, Process> processesInfo = new Dictionary<int, Process>();
    private static List<KeyValuePair<int, Process>> filteredProcesses;

    private static Dictionary<string, Action> sortingActions;

    private static Dictionary<string, Action> orderingActions;

    private static Dictionary<string, Action> showOptions;

    private static readonly Dictionary<int, ProcessInfo> processesActiveTime = new Dictionary<int, ProcessInfo>();

    private static Dictionary<string, Process> autoCompleteCollection;

    private static readonly IEnumerable<ProcessProperties> processPropertiesValues =
        Enum.GetValues(typeof(ProcessProperties)).Cast<ProcessProperties>();

    private static readonly Func<Process, int> GetMemoryUsageInMB = p => (int) (p.WorkingSet64 / (1024 * 1024));
    private static readonly Func<Process, TimeSpan> GetRuntimeOfProcess = p => DateTime.Now - p.StartTime;

    private static readonly Func<Process, TimeSpan> GetActiveTimeOfProcess =
        p => processesActiveTime[p.Id].TimeActive;

    private static readonly Func<TimeSpan, string> FormatTimeSpanToString =
        t => $"{(int) t.TotalHours} h : {t.Minutes} min";

    private static readonly Func<Process, string> GetStatusOfProcess =
        p => p.Responding ? "Active" : "Not responding";

    private static readonly Func<Func<Process, int>, int> GetAverageLengthOf =
        predicate =>
            filteredProcesses.Where(process => !process.Value.HasExited)
                .GetAverageLengthOf(p => predicate.Invoke(p.Value));

    private static readonly Func<Process, string> GetPathOfProcess = p => ProcessInspector.GetProcessPath(p);
    private static readonly Func<Process, bool> IsBackgroundProcess = p => ProcessInspector.IsBackgroundProcess(p);

    private static readonly Func<Process, int> GetInstaceCountOfProcess =
        p => Process.GetProcessesByName(p.ProcessName).Length;

    private static readonly Timer updateTimer = new Timer();
    private static readonly Timer focusTimeTimer = new Timer();

    private static ShowOptions showOption = default(ShowOptions);
    private static OrderBy currentOrder = default(OrderBy);
    private static string lastSortAction = string.Empty;

    private static Process selectedProcess;

    //it's functor<string> instead of string as selectedProcess is null at start
    //and we need actual reference so we have the latest value
    private static readonly Dictionary<ProcessProperties, Func<string>> processProperties = new Dictionary
        <ProcessProperties, Func<string>>
        {
            [ProcessProperties.Id] = () => selectedProcess.Id.ToString(),
            [ProcessProperties.Name] = () => selectedProcess.ProcessName,
            [ProcessProperties.InstancesRunning] = () => GetInstaceCountOfProcess(selectedProcess).ToString(),
            [ProcessProperties.Status] = () => GetStatusOfProcess(selectedProcess),
            [ProcessProperties.Owner] = () => ProcessInspector.GetProcessOwner(selectedProcess.Id),
            [ProcessProperties.TotalRuntime] = () => FormatTimeSpanToString(GetRuntimeOfProcess(selectedProcess)),
            [ProcessProperties.ActiveRuntime] =
            () => FormatTimeSpanToString(GetActiveTimeOfProcess(selectedProcess)),
            [ProcessProperties.StartTime] = () => selectedProcess.StartTime.ToString("g"),
            [ProcessProperties.MemoryUsage] = () => GetMemoryUsageInMB(selectedProcess) + " MB",
            [ProcessProperties.ProcessPath] = () => GetPathOfProcess(selectedProcess),
            [ProcessProperties.BackgroundProcess] = () => IsBackgroundProcess(selectedProcess).ToString(),
        };
}

MainForm.cs

public partial class MainForm : Form
{
    public MainForm()
    {
        InitializeComponent();
        InitializeSortingActions();
        InitializeOrderingActions();
        InitializeShowOptions();
        LoadProcesses();
        IntializeProcessList();
        InitializeInspectedProcessList();
        UpdateProcessList();

        updateTimer.Interval = 1000 * 10;
        updateTimer.Tick += UpdateTimer_Tick;

        focusTimeTimer.Interval = 1000;
        focusTimeTimer.Tick += FocusTimeTimer_Tick;
        focusTimeTimer.Start();
    }

    private void IntializeProcessList()
    {
        lvProcesses.Columns.Add("Name".ExtendWithEmptySpaces(GetAverageLengthOf(p => p.ProcessName.Length)));
        lvProcesses.Columns.Add("Status".ExtendWithEmptySpaces(GetAverageLengthOf(p => GetStatusOfProcess(p).Length)));
        lvProcesses.Columns.Add("Total Runtime".ExtendWithEmptySpaces(GetAverageLengthOf(p => FormatTimeSpanToString(GetRuntimeOfProcess(p)).Length)));
        lvProcesses.Columns.Add("Active Runtime".ExtendWithEmptySpaces(GetAverageLengthOf(p => FormatTimeSpanToString(GetActiveTimeOfProcess(p)).Length)));
        lvProcesses.Columns.Add("Start Time".ExtendWithEmptySpaces(GetAverageLengthOf(p => p.StartTime.ToString().Length) + 6));
        lvProcesses.Columns.Add("Memory Usage".ExtendWithEmptySpaces(GetAverageLengthOf(p => GetMemoryUsageInMB(p).ToString().Length)));
        lvProcesses.AutoResizeColumns(ColumnHeaderAutoResizeStyle.ColumnContent);
        lvProcesses.AutoResizeColumns(ColumnHeaderAutoResizeStyle.HeaderSize);
    }

    private void InitializeInspectedProcessList()
    {
        UpdateSearchOptions();
        lvInspectedProcess.Columns.Add("Property                   ");
        lvInspectedProcess.Columns.Add("Value                            ");
        lvInspectedProcess.AutoResizeColumns(ColumnHeaderAutoResizeStyle.ColumnContent);
        lvInspectedProcess.AutoResizeColumns(ColumnHeaderAutoResizeStyle.HeaderSize);
        lvInspectedProcess.Items.AddRange(
            processPropertiesValues.Select(
                value => CreateListViewRow(value.ToString().SplitOnCapitalLetters(), string.Empty)).ToArray());
    }

    private static Dictionary<string, Process> GetDistinctProcesses()
    {
        var distinctProcesses = new Dictionary<string, Process>();
        foreach (var filteredProcess in filteredProcesses)
        {
            if (!distinctProcesses.ContainsKey(filteredProcess.Value.ProcessName))
            {
                distinctProcesses.Add(filteredProcess.Value.ProcessName, filteredProcess.Value);
            }
        }
        return distinctProcesses;
    }

    private static IEnumerable<string> ExtractSearchOptions()
        => autoCompleteCollection.Values.Select(p => p.ProcessName);

    private void UpdateSearchOptions()
    {
        autoCompleteCollection = GetDistinctProcesses();
        tbSearchField.AutoCompleteCustomSource.AddRange(ExtractSearchOptions().ToArray());
    }

    private static void LoadProcesses()
    {
        processesActiveTime.RemoveAll((i, info) => info.Process.HasExited);
        processesInfo.RemoveAll((i, process) => process.HasExited);
        processesActiveTime.RemoveAll((i, info) => info.Process.HasExited);
        Process[] allProcesses = Process.GetProcesses();
        foreach (var process in allProcesses)
        {
            try
            {
                if (blackList.Contains(process) || processesInfo.ContainsKey(process.Id))
                {
                    continue;
                }
                //attempts to cause deny acess by the process.
                if (!process.HasExited)
                {
                    DateTime runtime = process.StartTime;
                }
                processesInfo.Add(process.Id, process);
                processesActiveTime.Add(process.Id, new ProcessInfo(process, new TimeSpan()));
            }
            catch (Win32Exception) { blackList.Add(process); }
            catch (InvalidOperationException) { }
        }
        showOptions[showOption.ToString()].Invoke();
    }

    private void InitializeSortingActions()
    {
        sortingActions = new Dictionary<string, Action>
        {
            ["Name"] = () => SortProcesses(p => p.ProcessName),
            ["Status"] = () => SortProcesses(p => p.Responding),
            ["Start Time"] = () => SortProcesses(p => p.StartTime),
            ["Total Runtime"] = () => SortProcesses(p => GetRuntimeOfProcess(p)),
            ["Memory Usage"] = () => SortProcesses(p => GetMemoryUsageInMB(p)),
            ["Active Time"] = () => SortProcesses(p => GetActiveTimeOfProcess(p))
        };

        foreach (var sortingAction in sortingActions)
        {
            cbSorting.Items.Add(sortingAction.Key);
        }
    }

    private void InitializeOrderingActions()
    {
        orderingActions = new Dictionary<string, Action>
        {
            [OrderBy.Ascending.ToString()] = () => currentOrder = OrderBy.Ascending,
            [OrderBy.Descending.ToString()] = () => currentOrder = OrderBy.Descending,
        };
        foreach (var orderingAction in orderingActions)
        {
            cbOrders.Items.Add(orderingAction.Key);
        }
    }

    private void InitializeShowOptions()
    {
        showOptions = new Dictionary<string, Action>
        {
            [ShowOptions.Default.ToString()] = () =>
            {
                showOption = ShowOptions.Default;
                filteredProcesses = processesInfo.ToList();
            },

            [ShowOptions.Active.ToString()] = () =>
            {
                showOption = ShowOptions.Active;
                filteredProcesses =
                    processesInfo.Where(p => !p.Value.HasExited && p.Value.Responding).ToList();
            },

            [ShowOptions.Foreground.ToString()] = () =>
            {
                showOption = ShowOptions.Foreground;
                filteredProcesses =
                    processesInfo.Where(p => !p.Value.HasExited && !IsBackgroundProcess(p.Value)).ToList();
            },
        };
        foreach (var option in showOptions)
        {
            cbShowOptions.Items.Add(option.Key);
        }
    }

    private void SortProcesses<T>(Func<Process, T> lambda)
        where T : IComparable
    {
        filteredProcesses.RemoveAll(p => p.Value.HasExited);

        switch (currentOrder)
        {
            case OrderBy.Descending:
                filteredProcesses.Sort(
                    (process1, process2) => lambda.Invoke(process2.Value).CompareTo(lambda.Invoke(process1.Value)));
                break;
            case OrderBy.Ascending:
                filteredProcesses.Sort(
                    (process1, process2) => lambda.Invoke(process1.Value).CompareTo(lambda.Invoke(process2.Value)));
                break;
        }
        UpdateProcessList();
    }

    private void UpdateProcessList()
    {
        updateTimer.Stop();
        updateTimer.Start();

        lvProcesses.Items.Clear();

        var rows = new List<ListViewItem>(filteredProcesses.Count);
        foreach (var processInfo in filteredProcesses)
        {
            try
            {
                TimeSpan runtime = GetRuntimeOfProcess(processInfo.Value);
                TimeSpan activeTime = GetActiveTimeOfProcess(processInfo.Value);
                rows.Add(
                    CreateListViewRow(
                        processInfo.Value.ProcessName,
                        GetStatusOfProcess(processInfo.Value),
                        FormatTimeSpanToString(runtime),
                        FormatTimeSpanToString(activeTime),
                        processInfo.Value.StartTime.ToString("g"),
                        GetMemoryUsageInMB(processInfo.Value) + " MB"));
            }
            catch (InvalidOperationException) { }
        }
        lvProcesses.BeginUpdate();
        lvProcesses.Items.AddRange(rows.ToArray());
        lvProcesses.EndUpdate();
    }

    private void RefreshList()
    {
        LoadProcesses();
        UpdateSearchOptions();
        if (!string.IsNullOrEmpty(lastSortAction))
        {
            sortingActions[lastSortAction].Invoke();
        }
        else
        {
            UpdateProcessList();
        }
    }

    private static ListViewItem CreateListViewRow(string name, string status, string runtime, string activeTime,
        string startTime, string memoryUsage)
    {
        return new ListViewItem(new[] {name, status, runtime, activeTime, startTime, memoryUsage});
    }

    private static ListViewItem CreateListViewRow(string propertyName, string value)
    {
        ListViewItem item = new ListViewItem(propertyName, 0);
        item.SubItems.Add(value);
        return item;
    }

    private void EditInspectedProcessSubItem(ProcessProperties processProperty, string value)
    {
        ListViewItem row = lvInspectedProcess.Items[(int)processProperty];
        row.SubItems[1] = new ListViewItem.ListViewSubItem(row, value);
    }

    #region Event handlers

    private void FocusTimeTimer_Tick(object sender, EventArgs e)
    {
        tbProcessCount.Text = filteredProcesses.Count.ToString();
        int activeProcessId = ProcessInspector.GetActiveProcessId();
        if (activeProcessId == -1)
        {
            return;
        }
        ProcessInfo activeProcess;
        if (processesActiveTime.TryGetValue(activeProcessId, out activeProcess))
        {
            try
            {
                activeProcess.TimeActive =
                    activeProcess.TimeActive.Add(new TimeSpan(0, 0, focusTimeTimer.Interval / 1000));
                if (activeProcess.TimeActive.Seconds == 0 && activeProcess.TimeActive.Minutes == 0 &&
                    activeProcess.TimeActive.TotalHours > 0)
                {
                    MessageBox.Show(
                        $@"You've spent {activeProcess.TimeActive.TotalHours} hours on '{activeProcess.Process
                            .ProcessName}'");
                }
            }
            catch (InvalidOperationException)
            {
                processesActiveTime.Remove(activeProcessId);
            }
        }
        else
        {
            RefreshList();
        }
    }

    private void UpdateTimer_Tick(object sender, EventArgs e)
    {
        if (cbEnableAutoUpdate.Checked)
        {
            RefreshList();
        }
    }

    private void bUpdate_Click(object sender, EventArgs e)
    {
        RefreshList();
    }

    private void ComboBoxSorting_SelectedIndexChanged(object sender, EventArgs e)
    {
        string itemText = ((Control)sender).Text;
        if (!string.IsNullOrEmpty(itemText))
        {
            lastSortAction = ((Control) sender).Text;
            sortingActions[lastSortAction].Invoke();
        }
    }

    private void ComboBoxOrders_SelectedIndexChanged(object sender, EventArgs e)
    {
        string itemText = ((Control)sender).Text;
        if (!string.IsNullOrEmpty(itemText) && itemText != showOption.ToString())
        {
            orderingActions[itemText].Invoke();
            if (!string.IsNullOrEmpty(lastSortAction))
            {
                sortingActions[lastSortAction].Invoke();
            }
        }
    }

    private void ComboBoxShowOptions_SelectedIndexChanged(object sender, EventArgs e)
    {
        string itemText = ((Control) sender).Text;
        if (!string.IsNullOrEmpty(itemText) && itemText != showOption.ToString())
        {
            showOptions[itemText].Invoke();
            if (!string.IsNullOrEmpty(lastSortAction))
            {
                sortingActions[lastSortAction].Invoke();
            }
            else
            {
                UpdateProcessList();
            }
        }
    }

    private void ProcessesListView_SelectedIndexChanged(object sender, EventArgs e)
    {
        if (lvProcesses.SelectedIndices.Count == 0)
        {
            return;
        }
        int processIndex = lvProcesses.SelectedIndices[0]; //only 1 item can be selected anyway
        selectedProcess = filteredProcesses[processIndex].Value;
        if (selectedProcess.HasExited)
        {
            RefreshList();
            selectedProcess = null;
        }
        else
        {
            tbSearchField.Text = selectedProcess.ProcessName;
        }
    }

    private void MainForm_Resize(object sender, EventArgs e)
    {
        if (WindowState == FormWindowState.Minimized && !ProcessTrackerIcon.Visible)
        {
            ProcessTrackerIcon.Visible = true;
            ProcessTrackerIcon.BalloonTipText = @"Minimized to tray";
            ProcessTrackerIcon.ShowBalloonTip(1000);
            ShowInTaskbar = false;
        }
    }

    protected override void OnFormClosing(FormClosingEventArgs e)
    {
        ProcessTrackerIcon.Visible = false;
        base.OnFormClosing(e);
    }

    private void ProcessTrackerIcon_MouseDoubleClick(object sender, MouseEventArgs e)
    {
        WindowState = FormWindowState.Normal;
        ShowInTaskbar = true;
        ProcessTrackerIcon.Visible = false;
    }

    private void ProcessTrackerIcon_MouseClick(object sender, MouseEventArgs e)
    {
        WindowState = FormWindowState.Normal;
        ShowInTaskbar = true;
        ProcessTrackerIcon.Visible = false;
    }

    private void bInspect_Click(object sender, EventArgs e)
    {
        if (selectedProcess == null)
        {
            MessageBox.Show(@"No process selected !");
            return;
        }
        if (selectedProcess.HasExited)
        {
            MessageBox.Show(@"Selected process has already been terminated !");
            RefreshList();
            return;
        }
        for (int i = 0; i < lvInspectedProcess.Items.Count; i++)
        {
            ProcessProperties processProperty = (ProcessProperties) i;
            EditInspectedProcessSubItem(processProperty, processProperties[processProperty].Invoke());
        }
    }

    private void bTerminate_Click(object sender, EventArgs e)
    {
        ApplyActionOnSelectedProcess(() =>
            {
                selectedProcess.Kill();
            },
            $"{selectedProcess.ProcessName} was successfully terminated",
            $"Failed terminating {selectedProcess.ProcessName}");
        selectedProcess = null;
        RefreshList();
    }

    private void bRefresh_Click(object sender, EventArgs e)
    {
        ApplyActionOnSelectedProcess(() => selectedProcess.Refresh(),
            $"{selectedProcess.ProcessName} was successfully refreshed",
            $"Failed refreshing {selectedProcess.ProcessName}");
    }

    private static void ApplyActionOnSelectedProcess(Action action, string successMessage, string failedMessage)
    {
        if (selectedProcess == null)
        {
            MessageBox.Show(@"No process selected");
            return;
        }
        try
        {
            action();
            MessageBox.Show(successMessage);
        }
        catch (Exception)
        {
            MessageBox.Show(failedMessage);
        }
    }

    private void SearchFieldTextBox_TextChanged(object sender, EventArgs e)
    {
        autoCompleteCollection.TryGetValue(tbSearchField.Text, out selectedProcess);
    }

    #endregion
}

Thank you for taking your time to read that.

\$\endgroup\$
1
  • \$\begingroup\$ Any chance you can share entire project? I would like to toy with it. \$\endgroup\$
    – Nikita B
    Commented Jan 11, 2017 at 7:49

3 Answers 3

5
\$\begingroup\$

Moving on to the last and longest part of the code, the actual form. Now here, we have quite some lines of code (452) and since I don't see a good reason to refactor the class into smaller ones (if you do please point that out in an answer)

Arguably, 452 lines of code in a single class (which, to make things worse, is partial) is good enough reason in itself. Another reason: it is not really UI layer's job to query windows api for process updates and track active process. This is a business logic, which should be separated from presentation (in case that one day you get fed up with Winforms evil ways and join the ranks of holy WPF warriors). You should extract it into separate service:

interface IProcessMonitor
{
    ProcessInfo ActiveProcess { get; }
    ProcessInfo[] Processes { get; }

    //request asynchronous update
    void ForceUpdate();

    event Action ActiveProcessChanged;
    event Action Updated;
}

or something along those lines. Or you can use Task<T>-based or IObservable<T>-based api instead just for the kicks. But the point is:

  • your MainForm should not do anything that is not directly related to UI. Generating rows based on some data is fine, generating the data itself - is not. So LoadProcesses, FocusTimeTimer_Tick and other similar methods will have to go.
  • you should not gather process information in UI thread. From the looks of it you are using Timer from Windows.Forms namespace, and this timer runs on UI thread. Which is why

    bottleneck here appears to be the functions that retrieve the process

    Call those function from timers that use threadpool (such as System.Timers.Timer) or from manually created tasks, and there will be no bottleneck.


Also, what instantly raises red flags is the sheer amount of various Functions, Functions of Functions, dictionaries of Functions, etc. gathered in one place. I mean come on

Func<Func<Process, int>, int>

this is nuts. :) Just create proper abstractions.

//converts general-purpose business object to Winforms-specific presentation format.
class ProcessInfoConverter
{
    public Dictionary<ProcessProperties, string> Convert(ProcessInfo info)
    {
        var res = new Dictionary<ProcessProperties, string>();
        res.Add(ProcessProperties.Id, info.Process.Id.ToString());
        res.Add(ProcessProperties.Name, info.Process.Name);
        res.Add(ProcessProperties.ActiveRuntime, FormatTimeSpan(info.ActiveRuntime));

        //etc.
        return res;
    }

    private string FormatTimeSpan(TimeSpan time)
    {
        return time.ToString("hh hours mm minutes");// or w/e
    }

    //etc.
}

No delegates involved and the code is much easier to navigate. Not to mention, that this will allow you to extract yet another massive code block from your oversized form.


It also feels like you are overusing static keyword. Why is selectedProcess static? Why are timers static? Why is list of processes static? Can they be made non-static, without modifying the rest of the code? If they can, then you should do so, it will greatly increase reusability and testability of your code. If they can't - then this is clearly a design issue. You should not rely on some global state in your code. The rule of a thumb is:

  • you can use static for simple immutable "constants"
  • you can use static for pure functions
  • in all other cases avoid using static (yes, I'm looking at you, Service Locator)

EDIT:

I still want to see your example usage

The usage is fairly straightforward. You inject the service into your form's constructor (or instantiate it there if you do not care much about IoC) and store it as a field, say _monitor. Then you just wire your UI to appropriate methods. For example, if we take the API above, you can refresh the list as follows:

public MainForm(IProcessMonitor monitor)
{
    ...

    _monitor = monitor;
    _converter = new ProcessInfoConverter();
    _monitor.Updated += OnUpdated;
    LoadProcesses();
}

private readonly IComparer<ProcessInfo> _sortingComparer;
private readonly IProcessMonitor _monitor;
private readonly ProcessInfoConverter _converter;

private void LoadProcesses()
{
    _monitor.ForceUpdate();
}

private void bUpdate_Click(object sender, EventArgs e)
{
    LoadProcesses();
}

private void OnUpdated()
{
    var items = _monitor.Processes
                        //filtering logic
                        .Where(Filter)
                        //sorting logic (use IComparer instead of delegates)
                        .OrderBy(_sortingComparer)
                        .Select(_converter.Convert)
                        .Select(GenerateRow)
                        .ToArray();

    //throw the changes back to UI thread
    BeginInvoke(new Action(() =>
         {
             lvProcesses.BeginUpdate();
             lvProcesses.Items.Clear();
             lvProcesses.Items.AddRange(items);
             lvProcesses.EndUpdate();
         }));
}

 private ListViewItem GenerateRow(Dictionary<ProcessProperties, string> properties) 
 {
     return new ListViewItem (...);
 }

 private bool Filter(ProcessInfo info) {...}

This implementation assumes, that

  • _monitor.ForceUpdate() is asynchronous call that starts a background job of fetching information about processes.
  • once information is loaded, _monitor.Processes property is set to a new value and Updated event triggers on background thread.

Note, that timers are actually gone from UI layer. It is IProcessMonitor's job now to look for changes and fire Updated event periodically.

Active process info can be refreshed in much the same way.

P.S. This was very much coded using notepad, so I hope I didn't make too many mistakes.

\$\endgroup\$
5
  • \$\begingroup\$ Excellent advice on static. I was thinking about this the other day and you've managed to sum it up far more eloquently than I managed! \$\endgroup\$
    – RobH
    Commented Jan 12, 2017 at 10:20
  • \$\begingroup\$ I'd like you to expand on the IProcessMonitor maybe with some examples. \$\endgroup\$
    – Denis
    Commented Jan 12, 2017 at 11:59
  • \$\begingroup\$ @denis, example of implementation or example of usage? \$\endgroup\$
    – Nikita B
    Commented Jan 12, 2017 at 13:39
  • \$\begingroup\$ Yes, I do got idea how it would work, but I still want to see your example usage. \$\endgroup\$
    – Denis
    Commented Jan 12, 2017 at 13:46
  • \$\begingroup\$ @denis, I've added a small example. \$\endgroup\$
    – Nikita B
    Commented Jan 12, 2017 at 14:50
7
\$\begingroup\$

Just a very small review to get you started - the code looks pretty good at first glance. You have nice small functions that seem to nicely split up the work. I'm also terrible at winforms so I won't try to suggest things around that.

Your extension method ExtendWithEmptySpaces seems to be functionally equivalent to the built in method String.PadRight.

This:

lvInspectedProcess.Columns.Add("Property                   ");

Would be better as:

lvInspectedProcess.Columns.Add("Property".PadRight(27));

This bit of code jumps out as not being in the same style as the rest of it:

string query = "SELECT ExecutablePath FROM Win32_Process WHERE ProcessId = " + process.Id;

That's for 2 reasons -

  1. Explicit type when the RHS is obvious
  2. string concatenation

I'd prefer it to look like:

var query = $"SELECT ExecutablePath FROM Win32_Process WHERE ProcessId = {process.Id}";

Another very minor note is that in English, unlike some languages, the punctuation should immediately follow the last word in the sentence. That means:

MessageBox.Show(@"No process selected !");

Should be:

MessageBox.Show(@"No process selected!");

That said, I would avoid using exclamation marks in messages to users. It suggests that the program is surprised something has happened (which it isn't) but it can also seem intimidating to less technically savvy users.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Silly me @Heslacher already mentionedPadRight in my previous question, but somehow I forgot to swap it. Thanks for the review, I swapped the string concatenation with interpolated string. \$\endgroup\$
    – Denis
    Commented Jan 10, 2017 at 13:20
5
\$\begingroup\$

There's not much to complain about. You've implemented all improvents so the performance is now very good. But hey! This is a good thing ;-)

What's left are just a few style things:

public static void RemoveAll<K, V>(this IDictionary<K, V> dict, Func<K, V, bool> match)

These K and V should be TKey and TValue.


var distinctProcesses = new Dictionary<string, Process>();
foreach (var filteredProcess in filteredProcesses)

You use vars. Not everywhere but finally somewhere :-D


[OrderBy.Ascending.ToString()]
[ShowOptions.Default.ToString()]

I'm not very fond of these ToStrings. Why not pure enums?


bInspect_Click

The code mixes the hungarian notation - this actually should be btnInspect_Click becasue b stands for bool - with the human notation:

ProcessTrackerIcon_MouseClick

and even mixed hungarian-human notation:

ComboBoxSorting_SelectedIndexChanged

preferable:

SortingComboBox_SelectedIndexChanged

if (cbEnableAutoUpdate.Checked)
{
    RefreshList();
}

This is not very pretty. The check-box should enable/disable the timer via its Enabled property and not prevent it from doint its job like that.

\$\endgroup\$

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