1
\$\begingroup\$

I'm a novice, self taught C# developer who's working on my first major program. The program I'm building is a system startup manager that is supposed to replace the stock startup manager that ships with Windows.

GitHub

You can find instructions on how to use it from the README.txt file, and to get an idea of how it looks, look into "SystemStartupProgram.PNG".

This program was originally made for private use but I figured I might as well let others use it (for free) when it's finished.

The basic functionality of my program is this; it reads a user input, searches for the program name from all directories on the pc and then writes the name of the program and its path to "utilities.txt", and adds both to a list called ProgramNames. Another way to add a program is to use the explorer for which there is a seperate button. Using this method instead, it won't look for a directory since it is already given by the explorer. It then adds the program in the same way.

Upon user login, it will read the program names / paths from "utilities.txt" and adds those to the list ProgramNames. When the user wants to launch programs through a button, it will go through the list of programs saved and launch each program separately.

To have an idea of what the program looks like visually, please refer to "SystemStartupProgram.PNG" found on Github, linked earlier.

Note: Apparently I could replace the AppendingThreads with Task.Run(--). I'm planning on making a seperate Windows service application for it, instead of running the .exe itself on startup. (This program would just be a GUI for the service version).

Also, I downloaded Puma Scan and it didn't find any security flaws. Should I trust its judgement?

I would love to get feedback on two things:

  1. How's the quality of the code, and are there ways to improve it? If so, how?
  2. Can you find any weak points that may cause trouble to end users? (Security flaws, things that could slow down the end users' computer, etc)

Parts of my program are missing comments, sorry about those parts.

namespace SystemStartupProgram
{

public partial class StartupManager : Form
{
    // Utilities
    Thread addToListThread = null;
    StreamWriter writer;
    StreamReader reader;
    private Thread appendingThread = null;
    private Thread setTextThread = null;
    private Boolean shouldChange = true;

    String defaultDirectory = "";

    // List contains names of all saved programs
    public static List<String> ProgramNames = new List<String>();

    public StartupManager()
    {
        InitializeComponent(); // Creates application

        // Set program's directory to path, where it's utilities are located
        var AppDataDirectory = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData);

        String subFolderDirectory = Path.Combine(AppDataDirectory, "StartupProgram");

        if (Directory.Exists(subFolderDirectory))
        {
            Directory.SetCurrentDirectory(subFolderDirectory);
        }
        else
        {
            Directory.CreateDirectory(subFolderDirectory);
            Directory.SetCurrentDirectory(subFolderDirectory);
        }

        SetDefaultDirectory(subFolderDirectory);

        if (File.Exists("utilities.txt"))
        {
            CheckForEmptyFileAndShowPrograms();
            SetStartupBox();
            SetStartUp();
            StartAppsOnStartup();

            if (File.Exists("folder.ico"))
            {
                this.Icon = Icon.ExtractAssociatedIcon(subFolderDirectory + "\\folder.ico");
            }
        }
        else
        {
            this.appendingThread = new Thread(() => AppendText("Text file could not be found! Creating new file.\n"));
            appendingThread.Start();
            using (writer = new StreamWriter("utilities.txt"))
            {
                if (LaunchCheckBox.Enabled == true)
                {
                    writer.WriteLine("false;startup;");
                }
            }
        }
    }


    /// <summary>
    /// All functions are located here
    /// </summary>

    #region All functions

    /// <summary>
    /// Reads file and saves all currently saved programs and their indexes
    /// to a String which is returns
    /// </summary>

    private static String ReadProgramInfoFromFile()
    {
        String line;
        int count = 0;
        StringBuilder savedPrograms = new StringBuilder();

        Boolean isFirstIT = true;

        try
        {
            StreamReader reader = new StreamReader("utilities.txt");

            while ((line = reader.ReadLine()) != null)
            {
                if (line.EndsWith(".exe") && !line.ToLower().StartsWith("c:\\"))
                {
                    if (isFirstIT) // On first cycle, add this text
                    {
                        savedPrograms.Append("Currently saved programs: \n\n");
                        isFirstIT = false;
                    }
                    savedPrograms.Append(count + ": " + line + "\n");
                    ProgramNames.Add(line);
                    count++;
                }
                else if (line.ToLower().StartsWith("c:\\"))
                {
                    savedPrograms.Append("Path: " + line + "\n\n");
                    ProgramNames.Add(line);
                }
            }
            savedPrograms.Append("---------------------------------------------------" +
                "-----------------------------------------------------------------\n\n");
            reader.Close();
        }
        catch { }

        return savedPrograms.ToString();
    }

    // Disables all controls
    private void DisableAllControls()
    {
        LaunchButton.Enabled = false;
        DeleteAllButton.Enabled = false;
        ClearButton.Enabled = false;
        ShowSavedButton.Enabled = false;
        SetButton.Enabled = false;
        DeleteButton.Enabled = false;
        ExplorerButton.Enabled = false;

        InputTextBar.Enabled = false;
        DeleteTextBox.Enabled = false;

        LaunchCheckBox.Enabled = false;
    }

    // Enables all controls
    private void EnableAllControls()
    {
        SetControlPropertyThreadSafe(LaunchButton, "Enabled", true);
        SetControlPropertyThreadSafe(DeleteAllButton, "Enabled", true);
        SetControlPropertyThreadSafe(ClearButton, "Enabled", true);
        SetControlPropertyThreadSafe(ShowSavedButton, "Enabled", true);
        SetControlPropertyThreadSafe(SetButton, "Enabled", true);
        SetControlPropertyThreadSafe(DeleteButton, "Enabled", true);
        SetControlPropertyThreadSafe(ExplorerButton, "Enabled", true);

        SetControlPropertyThreadSafe(InputTextBar, "Enabled", true);
        SetControlPropertyThreadSafe(DeleteTextBox, "Enabled", true);

        SetControlPropertyThreadSafe(LaunchCheckBox, "Enabled", true);
    }

    // Same as AddToList(String, String) but instead locates file directory
    // of the specified program first.
    public void AddToList(String name)
    {
        if (String.IsNullOrWhiteSpace(name))
        {
            this.appendingThread = new Thread(() => AppendText("Please specify a file name first!"));
            appendingThread.Start();
            return;
        }

        if (!ContainsName(name))
        {

            writer = File.AppendText("utilities.txt");

            if (name.EndsWith(".exe") && !name.Contains("*"))
            {
                this.appendingThread = new Thread(() => AppendText("Locating file directory...\n"));
                appendingThread.Start();

                List<String> paths = GetDirectory(@"C:\\", name);
                if (paths.Count() > 0)
                {
                    String _path = paths[0];

                    if (_path.ToLower().Equals((defaultDirectory + "\\systemstartupprogram.exe").ToLower()))
                    {
                        this.appendingThread = new Thread(() => AppendText("Can't add this program!"));
                        appendingThread.Start();
                        return;
                    }

                    name = _path.Substring(_path.LastIndexOf("\\") +1);

                    if (_path.Contains("\\\\"))
                    {
                        _path = _path.Remove(_path.IndexOf("\\\\"), 1);
                    }

                    ProgramNames.Add(name);
                    ProgramNames.Add(_path);

                    writer.WriteLine(name);
                    writer.WriteLine(_path);

                    writer.Close();

                    this.appendingThread = new Thread(() => AppendText("Program added: " + name));
                    appendingThread.Start();

                    Thread.Sleep(100);

                    this.appendingThread = new Thread(() => AppendText("Path : "
                        + _path + "\n"));
                    appendingThread.Start();

                    DeleteProgramFromRegistry(name, _path);

                    if (ProgramNames.Count > 10)
                    {
                        appendingThread = new Thread(() => AppendText("Warning: more than 5 programs set to start! (May cause system to slow down on startup)"));
                        appendingThread.Start();
                    }

                }
                else
                {
                    this.appendingThread = new Thread(() => AppendText("Program not found!\n"));
                    appendingThread.Start();
                }
            }
            else
            {
                if (name.Contains("*"))
                {
                    this.appendingThread = new Thread(() => AppendText("Name cannot contain asterisks!\n"));
                    appendingThread.Start();
                }
                else if (!name.EndsWith(".exe"))
                {
                    this.appendingThread = new Thread(() => AppendText("File must end in '.exe' !\n"));
                    appendingThread.Start();
                }
            }

            writer.Close();
        }
        else
        {
            this.appendingThread = new Thread(() => AppendText("This file is already saved!\n"));
            appendingThread.Start();
        }

        EnableAllControls();
    }

    // Adds program to list 'ProgramNames' and writes it to file.
    public void AddToList(String name, String path)
    {
        // Make sure name isn't empty or nothing but spaces
        if (String.IsNullOrWhiteSpace(name))
        {
            this.appendingThread = new Thread(() => AppendText("Please specify a file name first!"));
            appendingThread.Start();
            return;
        }

        // Check if program is already saved
        if (!ContainsName(name))
        {

            writer = File.AppendText("utilities.txt");

            if (name.EndsWith(".exe") && path.ToLower().StartsWith("c:\\"))
            {

                name = path.Substring(path.LastIndexOf("\\") + 1);


                ProgramNames.Add(name);
                ProgramNames.Add(path);

                writer.WriteLine(name);
                writer.WriteLine(path);

                writer.Close();

                this.appendingThread = new Thread(() => AppendText("Program added: " + name));
                appendingThread.Start();

                this.appendingThread = new Thread(() => AppendText("Path for '" + name + "': " + path + "\n"));
                appendingThread.Start();

                DeleteProgramFromRegistry(name, path);

                if (ProgramNames.Count > 5)
                {
                    appendingThread = new Thread(() => AppendText("Warning: more than 5 programs set to start! (May cause system to slow down on startup)"));
                    appendingThread.Start();
                }
            }
            else if (!name.EndsWith(".exe"))
            {
                this.appendingThread = new Thread(() => AppendText("File must end in '.exe' !\n"));
                appendingThread.Start();
            }
            else if (!path.ToLower().StartsWith("c:\\"))
            {
                this.appendingThread = new Thread(() => AppendText("Path invalid!\n"));
                appendingThread.Start();
            }
        }
        else
        {
            this.appendingThread = new Thread(() => AppendText("This program is already saved!\n"));
            appendingThread.Start();
        }

    }


    // Checks if given string is already written in file
    private Boolean ContainsName(String name)
    {

        using (StreamReader reader = new StreamReader("utilities.txt"))
        {
            String contents = reader.ReadToEnd();
            if (contents.Contains(name))
            {
                return true;
            }
        }
        return false;
    }

    /// <summary>
    /// Launches all saved applications in order by getting name & path from ProgramNames.
    /// </summary>
    private async void LaunchSavedPrograms()
    {
        String path = "";
        String name = "";

        // Make sure list isn't empty
        if (ProgramNames.Count > 0)
        {

            // Go through every application in the list
            for (int i = 0; i < ProgramNames.Count(); i += 2)
            {

                String tempname = ProgramNames[i].ToLower();
                tempname = Regex.Replace(tempname, @"\s+", "");

                String temppath = ProgramNames[i + 1].ToLower();
                temppath = Regex.Replace(temppath, @"\s+", "");

                // Make sure file is a .exe
                if (tempname.EndsWith(".exe")) {
                    name = ProgramNames[i];
                }

                // Make sure path is a path and ends in program name (= path is program's path)
                if (temppath.StartsWith("c:\\") && temppath.EndsWith(tempname)){
                    path = ProgramNames[i + 1];
                }

                if (tempname.Equals("microsoftedge.exe"))
                {
                    Process.Start("microsoft-edge:");
                }

                try
                {
                    // Start program
                    var psi = new ProcessStartInfo(name);
                    Directory.SetCurrentDirectory(path.Substring(0, path.LastIndexOf("\\")));
                    Process.Start(psi);

                    this.appendingThread = new Thread(() => AppendText("Program started: " + name + "\n" +
                        "Program path:     " + path + "\n"));
                    appendingThread.Start();

                    await Task.Delay(300);
                }
                catch (Win32Exception ex)
                {
                    this.appendingThread = new Thread(() => AppendText("Error starting program: " + ex.Message + "\n" +
                        "Failed program: " + tempname + ".\nProgram path: " + temppath + "\n"));
                    appendingThread.Start();

                    await Task.Delay(300);
                }
            }

            // After starting all apps, set current directory back to default
            Directory.SetCurrentDirectory(defaultDirectory);

            this.appendingThread = new Thread(() => AppendText("Loading complete.\n"));
            appendingThread.Start();
        }
    }

    // Gets the path of specified application (f.ex 'notepad.exe' output =  'c:/windows/notepad.exe')
    private List<String> GetDirectory(string path, string pattern)
    {
        var files = new List<String>();

        try
        {
            files.AddRange(Directory.GetFiles(path, pattern, SearchOption.TopDirectoryOnly));
            foreach (var directory in Directory.GetDirectories(path))
            {
                files.AddRange(GetDirectory(directory, pattern));
            }

        }
        catch (UnauthorizedAccessException) { }

        return files;
    }

    // Checks to see if file is empty and if not, sets TextArea text to program names / directories
    private void CheckForEmptyFileAndShowPrograms()
    {
        if (File.Exists("utilities.txt"))
        {
            String _true = "true;startup;";
            String _false = "false;startup;";
            // Reads file and determines if file is empty or not, sets text accordingly
            using (StreamReader reader = new StreamReader("utilities.txt"))
            {
                String contents = reader.ReadToEnd();
                if (contents.Contains(_true))
                {
                    contents = contents.Remove(contents.IndexOf(_true), _true.Length);
                }
                else if (contents.Contains(_false))
                {
                    contents = contents.Remove(contents.IndexOf(_false), _false.Length);
                }

                if (String.IsNullOrWhiteSpace(contents))
                {
                    setTextThread = new Thread(() => SetText("No programs saved!\n"));
                    setTextThread.Start();
                }
                else
                {
                    setTextThread = new Thread(() => SetText(ReadProgramInfoFromFile()));
                    setTextThread.Start();
                }
            }
        }
        else
        {
            appendingThread = new Thread(() => AppendText("Not found"));
            appendingThread.Start();
        }
    }

    // Sets the value for default directory
    private void SetDefaultDirectory(String directory)
    {
       defaultDirectory = directory;
    }

    // Writes a regedit or deletes it according to state of check box
    private void SetStartUp()
    {
        RegistryKey rk = Registry.CurrentUser.OpenSubKey
            ("SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Run", true);

        if (LaunchCheckBox.Checked)
        {
            rk.SetValue("SystemStartupProgram", (defaultDirectory + "\\SystemStartupProgram.exe"));
        }
        else
        {
            rk.DeleteValue("SystemStartupProgram", false);
        }
    }

    // Reads file and determines if startup box should be enabled or not
    private void SetStartupBox()
    {
        Boolean shouldCheck = false;

        using (reader = new StreamReader("utilities.txt"))
        {
            String line = reader.ReadLine();

            if (line != null)
            {
                if (line.EndsWith(";startup;"))
                {
                    if (line.StartsWith("true"))
                    {
                        shouldCheck = true;
                    }
                    else if (line.StartsWith("false"))
                    {
                        shouldCheck = false;
                    }
                }
            }
        }

        if (shouldCheck)
        {
            shouldChange = false;
            LaunchCheckBox.Checked = true;
        }
        else
        {
            shouldChange = false;
            LaunchCheckBox.Checked = false;
        }
    }


    /// <summary>
    ///  Functions to properly append text to TextArea
    /// </summary>

    #region Safe appending/setting functions
    private void AppendText(string text)
    {
        if (this.TextArea.InvokeRequired)
        {
            argReturner ar = new argReturner(AppendText);
            this.Invoke(ar, new object[] { text });
        }
        else
        {
            this.TextArea.AppendText(text + "\n");
        }
    }

    private void SetText(string text)
    {
        if (this.TextArea.InvokeRequired)
        {
            argReturner ar = new argReturner(SetText);
            this.Invoke(ar, new Object[] { text });
        }
        else
        {
            this.TextArea.Text = text;
        }
    }

    private void SetControlPropertyThreadSafe(Control control, string propertyName, object propertyValue)
    {
        if (control.InvokeRequired)
        {
            control.Invoke(new SetControlPropertyThreadSafeDelegate(SetControlPropertyThreadSafe), 
                new object[] { control, propertyName, propertyValue });
        }
        else
        {
            control.GetType().InvokeMember(propertyName, System.Reflection.BindingFlags.SetProperty,
                null, control, new object[] { propertyValue });
        }
    }

    private void ThreadProcSafe(String text)
    {
        this.AppendText(text);
    }

    private void ThreadProcSage(string text)
    {
        this.SetText(text);
    }

    delegate void argReturner(string text);
    delegate void SetControlPropertyThreadSafeDelegate(
        Control control, string propertyName, object propertyValue);

    #endregion Safe appending/setting functions

    /// <summary>
    /// Region ends
    /// </summary>


    // Launch saved applications upon startup
    private async void StartAppsOnStartup()
    {
        await Task.Delay(100);
        LaunchButton.PerformClick();
    }


    /// <summary>
    ///  Handles program deletion. Checks if line contains desired string, in which case
    ///  don't write it to new file. In other cases, write the current line to new file.
    ///  After writing, delete old file and rename new file to old file's name.
    /// </summary>
    private void DeletePrograms()
    {
        String line = "";
        String tempLine = "";

        String program = DeleteTextBox.Text;

        program = program.ToLower();
        program = Regex.Replace(program, @"\s+", "");

        DeleteTextBox.Text = "";

        try
        {
            StreamReader reader = new StreamReader("utilities.txt");
            writer = new StreamWriter("temp.txt");

            while ((line = reader.ReadLine()) != null)
            {
                tempLine = line.ToLower();
                tempLine = Regex.Replace(tempLine, @"\s+", "");

                if (!tempLine.Equals(program))
                {
                    writer.WriteLine(line);
                }
                else if (tempLine.Equals(program))
                {
                    // If the next line is the directory for program, do nothing
                    if ((line = reader.ReadLine()).ToLower().StartsWith("c:\\"))
                    {
                    }
                }
            }
            writer.Close();
            reader.Close();

            // Renaming & deleting
            File.Delete("utilities.txt");
            System.IO.File.Move("temp.txt", "utilities.txt");

        }
        catch (Exception ex)
        {
            this.appendingThread = new Thread(() => AppendText("Error: " + ex.Message));
            appendingThread.Start();
        }

        // Resets programNames list
        ProgramNames = new List<String>();

        // Refresh saved applications
        ShowSavedButton.PerformClick();
    }

    // Opens file explorer, once file is selected, add it to program list
    private void SelectFileWBrowser()
    {
        OpenFileDialog browser = new OpenFileDialog
        {
            Title = "Select files to use",
            InitialDirectory = @"c:\\",
            Filter = "All files (*.*)|*.*|All files (*.*)|*.*",
            FilterIndex = 2,
            RestoreDirectory = true
        };

        if (browser.ShowDialog() == DialogResult.OK)
        {
            String path = browser.FileName;

            String name = path.Substring(path.LastIndexOf("\\") +1, path.Length - path.LastIndexOf("\\") -1);

            var thread = new Thread(() => AddToList(name, path));
            thread.Start();
        }
    }

    private void DeleteProgramFromRegistry(String program, String programPath)
    {
        if (program.EndsWith(".exe"))
        {
            program = program.Substring(0, program.IndexOf(".exe"));
        }

        programPath = programPath.ToLower();
        programPath = Regex.Replace(programPath, @"\s+", "");

        if (programPath.Contains("\\\\"))
        {
            programPath = programPath.Remove(programPath.IndexOf("\\"), 1);
        }

        RegistryKey rk = Registry.CurrentUser.OpenSubKey
            ("SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Run", true);

        foreach (var k in rk.GetValueNames())
        {
            String directory = rk.GetValue(k).ToString().ToLower();

            directory = Regex.Replace(directory, @"\s+", "");


            if (!directory.Contains("\"")){
                directory = directory.Substring(0, directory.LastIndexOf(".exe") + 4);
            }
            else
            {
                directory = directory.Substring(0, directory.LastIndexOf(".exe") + 5);
            }

            if (directory.StartsWith("\"") && directory.EndsWith("\""))
            {
                directory = directory.Substring(directory.IndexOf("\"") +1, directory.LastIndexOf("\"") -1);
            }

            if (directory.Equals(programPath))
            {
                if (directory.Equals((defaultDirectory + "\\systemstartupprogram.exe").ToLower()))
                {
                    LaunchCheckBox.Checked = !LaunchCheckBox.Checked;
                    return;
                }

                rk.DeleteValue(k, false);
                return;
            }
        }
    }

    /// <summary>
    /// Region ends
    /// </summary>

    #endregion All functions


    /// <summary>
    ///  All GUI related utilities are located here
    /// </summary>

    #region GUI Utilities

    /// <summary>
    /// All utilities with a delete function are located here
    /// </summary>

    #region Delete utilities

    // Asks user for confirmation, deletes all entires in file and 'programNames'
    private void DeleteAllButton_Click(object sender, EventArgs e)
    {
        if (ProgramNames.Count() > 0)
        {
            DialogResult dResult = MessageBox.Show("Are you sure?", "Delete all programs", MessageBoxButtons.YesNo);
            if (dResult == DialogResult.Yes)
            {
                using (writer = new StreamWriter("temp.txt"))
                {
                    if (LaunchCheckBox.Checked)
                    {
                        writer.WriteLine("true;startup;");
                    }
                    else
                    {
                        writer.WriteLine("false;startup;");
                    }
                }

                // Renaming & deleting
                File.Delete("utilities.txt");
                File.Move("temp.txt", "utilities.txt");

                ShowSavedButton.PerformClick();
                ProgramNames = new List<String>();
            }
        }
        else
        {
            this.appendingThread = new Thread(() => AppendText("Error deleting: No programs saved!\n"));
            appendingThread.Start();
        }
    }

    // Delete specified program from list
    private void DeleteTextBox_KeyPress(object sender, KeyPressEventArgs e)
    {
        if (e.KeyChar == (char)ConsoleKey.Enter)
        {
            DeletePrograms();

        }
    }

    // Delete specified program from list
    private void DeleteButton_Click(object sender, EventArgs e)
    {
        DeletePrograms();
    }

    /// <summary>
    /// Region ends
    /// </summary>

    #endregion Delete utilities


    /// <summary>
    /// All utilities related to adding programs to list are here
    /// </summary>

    #region Add utilities

    // Adds specified name to 'programNames'
    public void SetButton_Click(object sender, EventArgs e)
    {
        String name = (InputTextBar.Text).ToLower();

        InputTextBar.Text = "";

        DisableAllControls();

        var thread = new Thread(() => AddToList(name));
        thread.Start();
    }

    // Adds specified name to 'programNames'
    public void InputTextBar_KeyPress(object sender, KeyPressEventArgs e)
    {
        if (e.KeyChar == (char)ConsoleKey.Enter)
        {
            String name = (InputTextBar.Text).ToLower();

            InputTextBar.Text = "";

            DisableAllControls();

            var thread = new Thread(() => AddToList(name));
            thread.Start();
        }
    }

    /// <summary>
    /// Region ends
    /// </summary>

    #endregion Add utilities


    /// <summary>
    /// All other utilities are located here (Miscellaneous)
    /// </summary>

    #region Miscellaneous utilities

    private void ExplorerButton_Click(object sender, EventArgs e)
    {
        SelectFileWBrowser();
    }

    // Launches all applications in 'programNames'
    private void LaunchButton_Click(object sender, EventArgs e)
    {

        ThreadStart refer = new ThreadStart(LaunchSavedPrograms);
        this.appendingThread = new Thread(() => AppendText("Loading applications...\n"));
        appendingThread.Start();

        Thread launchThread = new Thread(refer);
        launchThread.Start();

    }

    private void CancelButton_Click(object sender, EventArgs e)
    {
        addToListThread.Abort();
        writer.Close();
        EnableAllControls();
        this.appendingThread = new Thread(() => AppendText("Process cancelled."));
        appendingThread.Start();
    }


    // Shows all currently saved programs (in text file)
    private void ShowSavedButton_Click(object sender, EventArgs e)
    {
        if (File.Exists("utilities.txt"))
        {
            CheckForEmptyFileAndShowPrograms();
        }
    }



    // Clears text area from all text
    private void ClearButton_Click(object sender, EventArgs e)
    {
        TextArea.Text = "";
    }

    private void AboutToolStripMenuItem_Click(object sender, EventArgs e)
    {
        MessageBox.Show("A program by Zecuel\nVersion: 1.0", "About StartupManager");
    }

    private void ExitToolStripMenuItem_Click(object sender, EventArgs e)
    {
        Application.Exit();
    }

    private void LaunchCheckBox_CheckedChanged(object sender, EventArgs e)
    {
        if (shouldChange)
        {
            String line;
            String toWrite = "";

            if (LaunchCheckBox.Checked == false)
            {
                toWrite = "false;startup;";
                this.appendingThread = new Thread(() => AppendText("Program will no longer start on system startup."));
                appendingThread.Start();
            }
            else if (LaunchCheckBox.Checked == true)
            {
                toWrite = "true;startup;";
                this.appendingThread = new Thread(() => AppendText("Program is set to automatically start on system startup."));
                appendingThread.Start();
            }

            using (reader = new StreamReader("utilities.txt"))
            {
                using (writer = new StreamWriter("temp.txt"))
                {
                    writer.WriteLine(toWrite);
                    while ((line = reader.ReadLine()) != null)
                    {
                        if (!line.EndsWith(";startup;"))
                        {
                            writer.WriteLine(line);
                        }
                    }
                }
            }

            File.Delete("utilities.txt");
            File.Move("temp.txt", "utilities.txt");

            SetStartUp();
        }

        shouldChange = true;
    }

    private void HelpToolStripMenuItem_Click(object sender, EventArgs e)
    {
        MessageBox.Show
            ("Common applications and their .exe's :\n\n" +
            "Microsoft Edge: microsoftedge.exe\n" +
            "Gyazo: gystation.exe\n" +
            "Hearthstone: hearthstone beta launcher.exe",
            "Help with Startup Manager");
    }

    #endregion Miscellaneous utilities

    /// <summary>
    /// Region ends
    /// </summary>

    #endregion GUI utilities

    /// <summary>
    /// Region ends
    /// </summary>
}
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Please include the description in the question. Put everything here that is necessary to understand what your program is doing. \$\endgroup\$ – t3chb0t Apr 15 '18 at 18:17
  • \$\begingroup\$ "Also, I downloaded Puma Scan and it didn't find any security flaws. Should I trust its judgement?" No. If you made a mistake in the configuration it will miss half the flaws it's capable of detecting. Besides, unless you're going to ship your software, that level of security is not something you should be concerned about. \$\endgroup\$ – Mast Apr 15 '18 at 18:23
  • \$\begingroup\$ Added description. \$\endgroup\$ – Zecuel Apr 15 '18 at 18:26
7
\$\begingroup\$

This is quite a lot of code to review, I can't go in-depth on everything here. I'll review it as I scroll through.


Long and clear method names are good.


I see a decent attempt at separating things into methods, but you've still fallen into the trap of mixing responsibilities in a single method. Some methods are too long or too nested.

You've also created a monster class. The file weighs in at 1060 lines (as per GitHub), which is not acceptable. This needs to be separated into separate files, and, more importantly, separate classes!

A simple suggestion for different classes:

  • StreamHandler - Handles all the streams and writing to/reading from disk.
  • UtilityFileProcessor - Handles everything pertaining to "utilities.txt". This will probably use StreamHandler internally.
  • ReportFormatter - decides the format of the text that gets written to disk. Takes in data variables and formats them into what you want them to be. Will use StreamHandler internally to write to disk.

This is just a two-minute suggestion based on a glance at your code. The codebase is too big for me to make an accurate analysis here.


private Boolean shouldChange = true;

There are two options here.

  • This boolean is used locally somewhere where the name makes enough sense in context. In that case, declare it locally.
  • This boolean is used in multiple methods and cannot be placed locally, but then you should rename it so I can see what it's used for based on its name. Should what change?

    if (Directory.Exists(subFolderDirectory))
    {
        Directory.SetCurrentDirectory(subFolderDirectory);
    }
    else
    {
        Directory.CreateDirectory(subFolderDirectory);
        Directory.SetCurrentDirectory(subFolderDirectory);
    }

You can extract the duplicate line and invert the if to save some lines (and repetition):

if(!Directory.Exists(subFolderDirectory))
      Directory.CreateDirectory(subFolderDirectory);

Directory.SetCurrentDirectory(subFolderDirectory);

this.appendingThread = new Thread(() => AppendText("Text file could not be found! Creating new file.\n"));
appendingThread.Start();

I'm not quite sure why this is wrapped in a thread. Were you trying to use async/tasks? Threads and tasks, while similar at a glance, are two different beasts.


if (line.EndsWith(".exe") && !line.ToLower().StartsWith("c:\\"))
{
    if (isFirstIT) // On first cycle, add this text
    {
        savedPrograms.Append("Currently saved programs: \n\n");
        isFirstIT = false;
    }
    savedPrograms.Append(count + ": " + line + "\n");
    ProgramNames.Add(line);
    count++;
}
else if (line.ToLower().StartsWith("c:\\"))
{
    savedPrograms.Append("Path: " + line + "\n\n");
    ProgramNames.Add(line);
}

I don't like this. Why are .exe files that are not on the C drive treated differently? Why are files that are not .exes and not on the C drive ignored?

Similar to a previous comment, ProgramNames.Add(line); can be put after the if since it's used in both cases.

What you're doing here is processing a list and immediately storing a formatted string used for output. I suggest you separate the business logic form the presentation logic.
Store the files (or their filepaths) in savedPrograms. And only format the data when you print the value (to a file or the console).

Without that separation, you're going to struggle if you ever decide to change some formatting or output your results to several sources (e.g. a file and the console) but using different formats for each.


// Disables all controls
private void DisableAllControls()
{
    LaunchButton.Enabled = false;
    DeleteAllButton.Enabled = false;
    ClearButton.Enabled = false;
    ShowSavedButton.Enabled = false;
    SetButton.Enabled = false;
    DeleteButton.Enabled = false;
    ExplorerButton.Enabled = false;

    InputTextBar.Enabled = false;
    DeleteTextBox.Enabled = false;

    LaunchCheckBox.Enabled = false;
}

// Enables all controls
private void EnableAllControls()
{
    SetControlPropertyThreadSafe(LaunchButton, "Enabled", true);
    SetControlPropertyThreadSafe(DeleteAllButton, "Enabled", true);
    SetControlPropertyThreadSafe(ClearButton, "Enabled", true);
    SetControlPropertyThreadSafe(ShowSavedButton, "Enabled", true);
    SetControlPropertyThreadSafe(SetButton, "Enabled", true);
    SetControlPropertyThreadSafe(DeleteButton, "Enabled", true);
    SetControlPropertyThreadSafe(ExplorerButton, "Enabled", true);

    SetControlPropertyThreadSafe(InputTextBar, "Enabled", true);
    SetControlPropertyThreadSafe(DeleteTextBox, "Enabled", true);

    SetControlPropertyThreadSafe(LaunchCheckBox, "Enabled", true);
}

Why does one method use SetControlPropertyThreadSafe and the other does not? This doesn't make sense to me.

Also, you can combine this in a single method:

private void EnableAllControls(bool isEnabled)
{
    SetControlPropertyThreadSafe(LaunchButton, "Enabled", isEnabled);
    //and so on...
}

It might not be a bad idea to put all of these controls in a list, and then doing:

foreach(myControl in myControlList)
    SetControlPropertyThreadSafe(myControl , "Enabled", isEnabled);

Saves you some copy/pasting.


public void AddToList(String name)
{
    if (String.IsNullOrWhiteSpace(name))
    {
        this.appendingThread = new Thread(() => AppendText("Please specify a file name first!"));
        appendingThread.Start();
        return;
    }

You're mixing business logic and presentation logic again. The responsibility of the AddToList method should be kept simple: it either adds the item, or it tells you why it can't add the item (most commonly via an exception with a string message).

Where that message gets posted (to a file, to the console, ...) should not be decided by the AddToList method.


this.appendingThread = new Thread(() => AppendText("Program added: " + name));
appendingThread.Start();

Thread.Sleep(100);

this.appendingThread = new Thread(() => AppendText("Path : " + _path + "\n"));
appendingThread.Start();

Again, why are these all separate threads? Why are you sleeping inbetween the two threads?

What is the point of doing something multithreaded if you're then going to force your main thread to wait until the other thread is completed?
If that's what you want, why not simply have the main thread do all the work to begin with? That way, you know for sure that it will only do the next step once it's done with the first step.

Trying to fill in the blanks here, I suspect that you've been trying to implement a task/async methodology, but you've decided to use thread for some unexplained reason.


ProgramNames.Add(name);
ProgramNames.Add(_path);

Since you're adding both the name of a file and its filepath as two separate entries, that proves that "programnames" is not a correct name for this variable.

Why are you storying two different types of data in the same list? I'm not saying you're wrong, but I would expect a list named ProgramNames to only contain the names of programs, not their filepaths.

You're either going to have to rename the list to what it actually represents, or redesign the code to do this differently.


// Make sure list isn't empty
if (ProgramNames.Count > 0)
{
    // Go through every application in the list
    for (int i = 0; i < ProgramNames.Count(); i += 2)
    {

Minor nitpick: the if is not necessary here. The for will simply loop 0 times if there are no items in the list.
If you had e.g. returned a custom message if the list were empty, then it would've been relevant to first check if there were items in the list. But if this happens, this method doesn't do anything; so that is not the case here.


try
{
    // Start program
    var psi = new ProcessStartInfo(name);
    Directory.SetCurrentDirectory(path.Substring(0, path.LastIndexOf("\\")));
    Process.Start(psi);

    this.appendingThread = new Thread(() => AppendText("Program started: " + name + "\n" + "Program path:     " + path + "\n"));
    appendingThread.Start();

    await Task.Delay(300);
}
catch (Win32Exception ex)
{
                this.appendingThread = new Thread(() => AppendText("Error starting program: " + ex.Message + "\n" + "Failed program: " + tempname + ".\nProgram path: " + temppath + "\n"));
    appendingThread.Start();

    await Task.Delay(300);
}

After all the thread code, suddently await Task.Delay(300); appears. I would expect that you either stick with one and not the other. In this case, I suggest moving everything to an async/task approach.


AppendText("Error starting program: " + ex.Message + "\n" + "Failed program: " + tempname + ".\nProgram path: " + temppath + "\n")

So far, I haven't mentioned anything about your string concatenation, because it wa limited to two or three items being concatenated. But now you're growing it in size.

String are immutable, meaning they can't change after they've been created. Any change (e.g. in length) means that a new string must be created.

If you do "a"+"b"+"c", what happens is:

  • Create a 1 character string ("a")
  • Create a 1 character string ("b")
  • Create a 2 character strign ("ab")
  • Create a 1 character string ("c")
  • Create a 3 character string ("abc")

If you do "Error starting program:" + ex.Message + "\n" + "Failed program: " + tempname + ".\nProgram path: " + temppath + "\n", what happens is:

  • Create a 24 character string ("Error starting program: ")
  • Assuming ex.Message is 50 characters long, create a 74 character string ("Error starting program: THIS IS A LONG 50 CHARACTER MESSAGE")
  • Create a 1 character string ("\n")
  • Create a 75 character string ("Error starting program: THIS IS A LONG 50 CHARACTER MESSAGE\n")
  • Create a 16 character string ("Failed program: ")
  • Create a 91 character string ("Error starting program: THIS IS A LONG 50 CHARACTER MESSAGE\nFailed program: ")
  • Assuming tempname is 15 characters long, create a 106 character string ("Error starting program: THIS IS A LONG 50 CHARACTER MESSAGE\nFailed program: MYTEMPNAME")
  • Create a 17 character string (".\nProgram path: ")
  • ...

I hope you can see how quickly this goes overboard. There are much better alternatives:

The string builder:

StringBuilder sb = new StringBuilder();
sb.Append("Error starting program:");
sb.Append(ex.Message);
sb.Append("\nFailed program: ");
sb.Append(tempname);
sb.Append(".\nProgram path: ");
sb.Append(temppath);
sb.Append("\n");

var myString = sb.ToString();

The string format:

var myString = String.Format("Error starting program:{0}\nFailed program: {1}.\nProgram path: {2}\n", ex.Message, tempname, temppath);

String.Format has recently come into disuse because of the shorter (and similar) string interpolation feature:

var myString = $"Error starting program:{ex.Message}\nFailed program: {tempname}.\nProgram path: {temppath}\n";

All three method make a string variable once. No hassle, no memory explosions, no performance dips. Use them and love them.


I constantly see "utilities.txt" used in the code. You have a few global variables; yet this filename is not one of them?

Just think of how much effort it would take if a user of your application wants to name their file something else. You'd spend quite some times tracking down all occurrences of the file name.


Many of your methods all spend some time and effort into formatting string and appending them to a writer.

As someone who's just read through your codebase; I still have no idea what your final file will look like, because all of the output logic is spread across your codebase.

Because you're using a streamwriter (which is a good thing!), it's easy to fall into the trap of having every method access the streamwriter whenever they please. But this will become a maintenance hell if your finished application is messing up the output and you want to debug why that is happening. Prepare to step through all of your code.

\$\endgroup\$
  • 1
    \$\begingroup\$ You don't even need if(!Directory.Exists(subFolderDirectory)): docs.microsoft.com/en-us/dotnet/api/… says "Creates all directories and subdirectories in the specified path unless they already exist." \$\endgroup\$ – BCdotWEB Apr 23 '18 at 14:00
  • \$\begingroup\$ Hello everyone! As this post went without any answers for a week or so i assumed it was dead and didn't bother updating this. I've made numerous changes in the past week, most of them mentioned in the answer above. I can upload the new code if you wish, but thanks for the detailed answer anyway! \$\endgroup\$ – Zecuel Apr 24 '18 at 14:32
  • \$\begingroup\$ Update; I've now fixed everything mentioned here, this was a tremendous help, thank you! \$\endgroup\$ – Zecuel Apr 28 '18 at 20:26

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.