wondering if there is an efficient way to shorten this code

Question

So my program is going to end up being fairly large and I don't want a whole lot of code that could just be shortened. Here is one instance I am looking for some tips on:

private void bookComboBox_SelectedIndexChanged(object sender, EventArgs e)
{
    string books = null;
    // sets books to the clicked item
    books = bookComboBox.SelectedItem.ToString();
    selectedPictureBox.Visible = true;
    // Loads string to list box and image to selectedPictureBox when programming is selected
    if (books == "Programming")
    {
        bookListBox.Items.Clear();
        selectedPictureBox.Image = Image.FromFile("programming.png");
        bookListBox.Items.Add("Visual Basic");
        bookListBox.Items.Add("Java");
        bookListBox.Items.Add("C#");
    }
    // Loads string to list box and image to selectedPictureBox when Networking is selected
    else if (books == "Networking")
    {
        bookListBox.Items.Clear();
        selectedPictureBox.Image = Image.FromFile("networking.png");
        bookListBox.Items.Add("LAN Networks");
        bookListBox.Items.Add("Windows Networking");
        bookListBox.Items.Add("More About Networking");
    }
    // Loads string to list box and image to selectedPictureBox when Web is selected
    else if (books == "Web")
    {
        bookListBox.Items.Clear();
        selectedPictureBox.Image = Image.FromFile("html.png");
        bookListBox.Items.Add("Web Programming");
        bookListBox.Items.Add("JavaScript");
        bookListBox.Items.Add("ASP");
    }
}

The code works fine but I was just hoping to get some tips on shortening this code if necessary, any input is appreciated.

Answer 1

Assuming you can use C# 7's new Tuples:

private Dictionary<string, (string image, List<string> books)> books = new Dictionary<string, (string image, List<string> books)>
{
    { "Programming", ("programming.png", new List<string> { "Visual Basic", "Java", "C#"} ) },
    { "Networking", ("networking.png", new List<string> {"LAN Networks", "Windows Networking", "More About Networking"}) },
    { "Web", ("html.png", new List<string> {"Web Programming", "Javascript", "ASP"}) }
};

private void bookComboBox_SelectedIndexChanged(object sender, EventArgs e)
{
    // sets books to the clicked item
    string book = bookComboBox.SelectedItem.ToString();
    selectedPictureBox.Visible = true;

    if (books.Keys.Contains(book))
    {
        bookListBox.Items.Clear();
        selectedPictureBox.Image = Image.FromFile(books[book].image);
        foreach(var b in books[book].books)
        {
           bookListBox.Items.Add(b);
        }
    }
}

But a class is likely even better:

public class BookGroup
{
    public string ImagePath {get;set;}
    public List<string> Books {get;}

    public BookGroup(string imagePath, params string[] books)
    {
        ImagePath = imagePath;
        Books = new List<string>(books.Length);
        Books.AddRange(books);
    }
}

Which isn't all that different to use right now, but it formalizes some things that might make this code easier to work with down the road, and it's possible if you can't use Tuples yet. I might also have a Book class by itself, just for fun, but for now:

private Dictionary<string, BookGroup> books = new Dictionary<string, BookGroup>
{
    { "Programming", new BookGroup("programming.png", "Visual Basic", "Java", "C#")},
    { "Networking", new BookGroup("networking.png","LAN Networks", "Windows Networking", "More About Networking") },
    { "Web", new BookGroup("html.png", "Web Programming", "Javascript", "ASP") }
};

private void bookComboBox_SelectedIndexChanged(object sender, EventArgs e)
{
    // sets books to the clicked item
    string book = bookComboBox.SelectedItem.ToString();
    selectedPictureBox.Visible = true;

    if (books.Keys.Contains(book))
    {
        bookListBox.Items.Clear();
        BookGroup bg = books[book];
        selectedPictureBox.Image = Image.FromFile(bg.ImagePath);
        foreach(var b in bg.Books)
        {
           bookListBox.Items.Add(b);
        }
    }
}

Regardless, I'd definitely have a way to load these from a text file... likely a csv, or maybe even a small in-process database, so that I could update this listing without having to recompile or distribute new program code. And, with that in mind, in order to fit this data in a single structure in a single file, I'd likely also repeat the image and type with each book, so that my csv data looks like this:

Topic,Image,Title
Programming,programming.png,"Visual Basic"
Programming,programming.png,"Java"
Programming,programming.png,"C#"
Networking,networking.png,"LAN Networks"
Networking,networking.png,"Windows Networking"
Networking,networking.png,"More About Networking"
Web,html.png,"Web Programming"
Web,html.png,"Javascript"
Web,html.png,"ASP"

That changes the whole character of the code. I'm a bit biased, but I'd likely use this CSV Parser, and again assuming Tuples I'd produce something like this:

private List<(string Topic, string ImagePath, string Title)> books;

//In the form load code:
books = EasyCSV.FromFile("bookData.csv").Select(b => (b[0], b[1], b[2])).ToList();

//and finally, in the original selectindexchanged method:
private void bookComboBox_SelectedIndexChanged(object sender, EventArgs e)
{
    string topic = bookComboBox.SelectedItem.ToString();
    selectedPictureBox.Visible = true;

    var items = books.Where(b => b.Topic == topic).ToArray();
    if(items.Length > 0)
    {
        bookListBox.Items.Clear();
        selectedPictureBox.Image = Image.FromFile(items[0].ImagePath);
        bookListBox.Items.AddRange(items);
    }
}

Answer 2

Make objects and use databindings.

public class Book
{
    public BookType BookType { get; set; }
    public string Name { get; set; }
    public string Image { get; set; }
}

public enum BookType
{
    Programming,
    Networking,
    Web,
}

public partial class Form1 : Form
{
    private readonly List<Book> _books = new List<Book>
    {
        new Book { Image = "programming.png", BookType = BookType.Programming, Name = "VB" },
        new Book { Image = "programming.png", BookType = BookType.Programming, Name = "Java" },
        new Book { Image = "programming.png", BookType = BookType.Programming, Name = "C#" },
        new Book { Image = "networking.png", BookType = BookType.Networking, Name = "LAN Networks" },
        new Book { Image = "networking.png", BookType = BookType.Networking, Name = "Windows Networking" },
        new Book { Image = "networking.png", BookType = BookType.Networking, Name = "More About Networking" },
        new Book { Image = "html.png", BookType = BookType.Web, Name = "Web Programming" },
        new Book { Image = "html.png", BookType = BookType.Web, Name = "Javascript" },
        new Book { Image = "html.png", BookType = BookType.Web, Name = "ASP" },
    };

    public Form1()
    {
        InitializeComponent();
    }

    private void Form1_Load(object sender, EventArgs e)
    {
        var bookTypes = _books.GroupBy(b => b.BookType).Select(g => g.Key).ToList();

        this.cboBookTypes.DataSource = bookTypes;
    }

    private void cboBookTypes_SelectedIndexChanged(object sender, EventArgs e)
    {
        var bookType = (BookType)this.cboBookTypes.SelectedItem;
        var books = _books.Where(b => b.BookType == bookType).ToList();
        var img = books.First().Image;

        this.imgBook.Image = Image.FromFile(img);
        this.lstBooks.DataSource = books;
        this.lstBooks.DisplayMember = "Name";

    }
}

Answer 3

Create a class to represent a book list:

public class BookList 
{
  public string ImageName { get; set; }
  public List<string> Items { get;set; }
}

Then create a dictionary to hold these items:

Dictionary<string, BookList> bookLists = new Dictionary<string, BookList> 
  {
    { 
       "Programming", 
       new BookList { ImageName = "programming.png", Items = new List<string> { ... } }
    }
    ...
  };

Then modify your if statements to:

if (bookLists.ContainsKey(books))
{
  bookListBox.Items.Clear();
  selectedPictureBox.Image = Image.FromFile(bookLists[books].ImageName);
  foreach (var b in bookLists[books].Items)
  {
     bookListBox.Items.Add(b);
  }
}

Answer 4

I do not have visual studio, so giving you the points/suggestions to improve on.

• switch should be preferred over if-elseif.
• bookListBox.Items.Clear(); and selectedPictureBox.Image out of if block. Use a variable to set the image file name.

Answer 5

I would suggest to keep all the data in a configuration object and then iterate through that data when performing checks and assignments.

I would create a separate class to hold data for each book: name, picture file name and check box items string array.

Then I would create a list of that object and assign all the data manually on form initialization.

After that, in SelectedIndexChanged event handler, I would iterate (for loop) on each item and check if the book name matched. If it did, then I would use data from that object and then break; the loop.

Answer 6

I think you should create a class that represents book category. Then, you could simply iterate through all the category lists and extract the necessary information, like this:

string books = null;
books = bookComboBox.SelectedItem.ToString();
selectedPictureBox.Visible = true;

for (int i = 0; i < categories.Count; i++) {
    if (books == categories[i].Name) {
        bookListBox.Items.Clear();
        selectedPictureBox.Image = Image.FromFile(categories[i].ImagePath);
        for (int j = 0; j < categories[i].Items.Count; j++) {
             bookListBox.Items.Add(categories[i].Items[j]);
        }        
    }
}

Answer 7

If you are talking about the length of the code, I would suggest using switch-case-break-default construct Switch the books variable. This wont improve the performance though