Keeping the main form tidy - conventions

Question

This is not a question about coding per se but about good practice in code structure. I'm currently building a WinForms application and already after a few hours in, my main form contains 130 lines of code. That might not be much but this only includes event handling - since I tried to avoid this exact situation by having seperate class files for basically everything ... but right now all the controls and events just make my main code pretty hard to read.

Now this is a topic I could find surprisingly little on and I do have some ideas on how to tackle this, like creating custom controls and splitting up the form into big portions. Is there a sort of best practice for this? How do you keep your main form clean when 80% of user interaction takes place here? Also is there a basic guideline on how to structure a project (not code) you can recommend?

(Hope this qualifies as a valid question)

Thanks!

EDIT: I decided to add in the code. See anything redundand?

public partial class MainForm : Form
{
    string currentFilter = "all";

    public MainForm()
    {
        InitializeComponent();
    }
    private void MainForm_Load(object sender, System.EventArgs e)
    {
        RefreshGenres();
        RefreshMovies(currentFilter);
    }

    private void addToolStripMenuItem_Click(object sender, System.EventArgs e)
    {
        var fAdd = new AddNewForm();

        fAdd.SetDesktopLocation(MousePosition.X, MousePosition.Y);
        fAdd.ShowDialog();
    }

    private void refreshToolStripMenuItem_Click(object sender, System.EventArgs e)
    {
        RefreshGenres();
        RefreshMovies(currentFilter);
    }

    private void addCategoryToolStripMenuItem_Click(object sender, System.EventArgs e)
    {
        tvCategories.Nodes.Add(new TreeNode("category"));
        tvCategories.Nodes[tvCategories.Nodes.Count - 1].BeginEdit();
    }
    private void tvCategories_AfterLabelEdit(object sender, NodeLabelEditEventArgs e)
    {
        var genre = e.Label;
        var writer = new Writer();
        writer.AddGenre(e.Label);

    }


    private void everythingToolStripMenuItem_Click(object sender, System.EventArgs e)
    {
        EraseData("all");
    }
    private void clearGenres_Click(object sender, System.EventArgs e)
    {
        EraseData("genres");
    }
    private void clearMovies_Click(object sender, System.EventArgs e)
    {
        EraseData("movies");
    }
    private void EraseData(string eraseThis)
    {
        DialogResult r = MessageBox.Show("Are you sure?\nLost data can NOT be retrieved.", "Clear Data", MessageBoxButtons.YesNo, MessageBoxIcon.Warning);

        var cmdText = "";

        if (r == DialogResult.Yes)
        {
            switch (eraseThis)
            {
                case "all":
                    cmdText = "TRUNCATE TABLE MOVIES GENRES";
                    break;

                case "movies":
                    cmdText = "TRUNCATE TABLE MOVIES";
                    break;

                case "genres":
                    cmdText = "TRUNCATE TABLE GENRES";
                    break;
            }
            conn.Open();
            using (var cmd = conn.CreateCommand())
            {
                cmd.CommandText = cmdText;
                cmd.ExecuteNonQuery();
            }
            conn.Close();
        }
    }


    private void RefreshGenres()
    {
        tvCategories.Nodes.Clear();

        var reader = new Reader();
        var genres = reader.GetGenreList();

        foreach (string str in genres)
        {
            tvCategories.Nodes.Add(str);
        }
    }
    private void RefreshMovies(string filter)
    {
        lvMovies.Items.Clear();

        var reader = new Reader();
        var movies = reader.GetMovieList(filter);

        foreach (ListViewItem item in movies)
        {
            lvMovies.Items.Add(item);
        }
        reader.conn.Close();

    }

    private void tvCategories_NodeMouseClick(object sender, TreeNodeMouseClickEventArgs e)
    {
        currentFilter = e.Node.Text;
        RefreshMovies(currentFilter);
    }
}

Answer 1

I think that the problem here is not the length of your code, but the fact that you are mixing up many different things and that you are not dealing with your events in a consistent way. These are the main things that I would change:

EraseData: this method opens and uses a DB connection, and this is something that you usually should avoid (changing something in the DB would cause you to modify the UI code), your WinForm code should not know where the data it is exposing come from. Here you'd be better adding an EraseData to your Writer class.

RefreshMovies: it is quite different from RefreshGenres, even if both of these methods do essentially the same thing: they are getting lists of data which are added to some UI controls. RefreshGenres seems ok, but looking at RefreshMovies you immediately see that you are closing a connection that was not opened in this method. Probably you opened it in the Reader class, but that's the place where you should have closed it, too. In fact, it'd be better for conn to be private (remember, UI doesn't have to know whether data come from a DB, a text file, or a user input). Furthermore GetGenreList of Reader returns a list of strings, which is ok, but GetMovieList returns a list of ListViewItem, which is not good because this is something strongly related to your specific implementation of UI. This means that your implementation of Reader could not be used in a WPF or web application. the ListViewItems should be created in RefreshMovies using plain data got from the Reader.