Help refactoring Winform combo box.

Robb Hill
Robb Hill used Ask the Experts™
on
I have the following winform.

Essentially I need to refactor this code as I am getting some bad behavior..please assist.

1) I was hoping to put a default value in each of these two combo boxes...something like select....basically some words so it doesnt default to a real value.

2)  The form combo box updates the request dropdown.  
I need to better validate this ....and also the records just append on each call of the selected index change.  
I know somewhere i need to  clear this object.

3)  It would be cool to have the option on the request ID combo to auto type in such a way that it gets filled by the selection of the form combobox and then you can auto type to get the results faster in the request combo box.

please help and any other best practices I would gladly appreciate.  

Here is complete code behind of form UI

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using InfoPathAttachmentEncoding;
using AttachmentsLibrary;
using System.Diagnostics;

namespace SP2007AttachmentArchival
{
    public partial class frmDownLoadAttachments : Form
    {
        private StringBuilder sb = new StringBuilder();
        List<Attachments> attachment = new List<Attachments>();

        public frmDownLoadAttachments()
        {
            InitializeComponent();

            LoadFormList();

            LoadRequestIdList();          

        }

        private void LoadFormList()
        {
            attachment = SqliteDataAccess.LoadFormId();

            WireUpFormList();
        }

        private void WireUpFormList()
        {
            cmbFormType.DisplayMember = "Form";
            cmbFormType.ValueMember = "Id";
            cmbFormType.DataSource = attachment;
        }

        private void LoadRequestIdList()
        {
            attachment = SqliteDataAccess.LoadRequestId(cmbFormType.Text);

            WireUpRequestIdList();
        }
        
        private void WireUpRequestIdList()
        {
            cmbRequestId.DisplayMember = "RequestId";
            cmbRequestId.ValueMember = "Id";
            cmbRequestId.DataSource = attachment;
        }
        
        private void BtnDLAttachments_Click(object sender, EventArgs e)
        {
            sb.Clear();
            bool bContinue = true;
            progressBar1.Value = 0;
            this.Cursor = Cursors.WaitCursor;

            if (txtDLFolder.Text.Length == 0)
            {
                sb.AppendLine("Please select a valid Download Folder");
                bContinue = false;
            }           
            if (string.IsNullOrEmpty(cmbFormType.Text))
            {
                sb.AppendLine("Please select Form Type");
                bContinue = false;
            }
            if (string.IsNullOrEmpty(cmbRequestId.Text))
            {
                sb.AppendLine("Please select Request ID");
                bContinue = false;
            }

            if (bContinue)
            {
                List<Attachments> attachment = new List<Attachments>();

                attachment = SqliteDataAccess.LoadAttachments(cmbFormType.Text, cmbRequestId.Text);

                if (attachment.Count() == 0)
                {
                    sb.AppendLine(String.Format("There are no attachments for {0}", cmbRequestId.Text));
                    txtResponse.Text = sb.ToString();
                }
                else
                {
                    InfoPathAttachmentDecoder ipad;
                    progressBar1.Maximum = attachment.Count();
                    int i = 0;
                    foreach (var item in attachment)
                    {
                        i++;
                        ipad = new InfoPathAttachmentDecoder(item.Document);
                        sb.AppendLine(ipad.Filename);
                        ipad.SaveAttachment(txtDLFolder.Text);
                        txtResponse.Text = sb.ToString();
                        progressBar1.Value = i;
                        Application.DoEvents();
                    }
                    Process.Start(txtDLFolder.Text + "\\");

                }

            }
            else
                txtResponse.Text = sb.ToString();

            this.Cursor = Cursors.Default;         

           
        }

        private void btnDLFolder_Click(object sender, EventArgs e)
        {
            folderBrowserDialog1.ShowDialog();
            txtDLFolder.Text = folderBrowserDialog1.SelectedPath;

        }

        private void CmbFormType_SelectedIndexChanged(object sender, EventArgs e)
        {

            attachment = SqliteDataAccess.LoadRequestId(cmbFormType.Text);

            WireUpRequestIdList();

        }
    }

}

Open in new window

Comment
Watch Question

Do more with

Expert Office
EXPERT OFFICE® is a registered trademark of EXPERTS EXCHANGE®
Commented:
The simplest method for #1 is just to insert a default value at the beginning of the list - a dummy entry that has an invalid ID, like 0:
ti.Insert(0, initialItem);

private void WireUpFormList()
        {
            // Inject a "Select a value..." dummy option at  the top
            if((attachment.Count > 0) && (attachment[0].Id == 0))
            {
              attachment.Insert(0, new Attachments()
              {
                Id = 0,
                Form = "Select a value..."
              });
            }

            cmbFormType.DisplayMember = "Form";
            cmbFormType.ValueMember = "Id";
            cmbFormType.DataSource = attachment;
        }

Open in new window


When you act upon the selection, just check the ID of the selected object to make sure it's greater than 0.

#2 - I don't understand what you're asking. If you have multiple, different questions, you might want to open up separate questions to avoid confusion.

#3 - Also not fully sure about what you're asking here, although it sounds like the default way that combo boxes work...
Robb HillSenior .Net Full Stack Developer

Author

Commented:
The dropdown for requestid is not clearing so i get dups.    So if I click form again I get appended records on the combo box
Robb HillSenior .Net Full Stack Developer

Author

Commented:
I wanted to be able to type in combo box ...like autocomplete
C++ 11 Fundamentals

This course will introduce you to C++ 11 and teach you about syntax fundamentals.

Commented:
If you're getting dupes, it's not from the code you've shown. My guess is that your SqliteDataAccess.LoadAttachments() method is maybe caching, then on the 2nd run it pulls from cache and appends results or something?
Robb HillSenior .Net Full Stack Developer

Author

Commented:
Do you think it's because it's called on the form and on selected index without and clear?
Robb HillSenior .Net Full Stack Developer

Author

Commented:
Don't see how it caches.   It's just a list bound to the combobox
Commented:
Also, don't do this:
Application.DoEvents();

I'm assuming you're doing that because without it, your progress bar didn't seem to be visually updating. However, that's the wrong way to do it.

Application.DoEvents() can be a very intensive call and can result in unwanted UI behavior sometimes. Basically, imagine a post office that collects outgoing mail in a box and then ships off the box at certain times of the day. That's sort of what the application does - it drops these various requests into a big "things-to-do" bucket, and then .NET figures out the appropriate time to handle them.

When you call Application.DoEvents(), it's like telling .NET to process EVERY request in that "to-do" bucket. Sometimes that bucket can hold a lot of requests, sometimes just a few, but it takes time to check the bucket and execute it, and standard UI behavior expects things to be run in a certain, standard way. When you force that bucket to be executed/emptied, it can result in some unexpected order-of-events - things you might not want happening (depends on your code).

The appropriate way to ensure that a control is visually updated when you're inside a loop or some other CPU-intensive task, is to first call Invalidate() on the control. This tells .NET that its current idea of how the control SHOULD look is now invalid and it needs to update what that control looks like. Then you call Refresh() on the control - this forces .NET to just repaint that one control.

So in your code:
                        progressBar1.Value = i;
                        // Application.DoEvents(); // <-- Comment this out and add the below 2 lines
                        progressBar1.Invalidate();
                        progressBar1.Refresh();

Open in new window

Commented:
It's hard to say for sure about the caching - I can't see the code for SqliteDataAccess.LoadAttachments, so I'm guessing here.

Based on the fact that you clear the attachment list when you click the button, it should start fresh each time. The only thing I can think of that might impact it is that you're defining a local variable that matches a class property:

Line 19: List<Attachments> attachment = new List<Attachments>();
Line 84: List<Attachments> attachment = new List<Attachments>();

Line 84 shouldn't declare the type again. It should just be:
attachment = new List<Attachments>();

...or just:
attachment.Clear();
Robb HillSenior .Net Full Stack Developer

Author

Commented:
Thank you.  I will try in the am
Robb HillSenior .Net Full Stack Developer

Author

Commented:
All of your answers were spot on!

Thank you so much!  Helped me and taught me a few things at the same time.  Cannot beat that:)

Commented:
Glad the info helped. My guess is that you'll soon be getting to the point of trying out / using multithreading. It's usually the next step in projects where you have long-running tasks and UI (assuming this, since you have a progress bar). Just my proactive recommendations:

1. Learn to use the Task, async, and await concepts for multithreading - lots of good articles online on this topic, and Stephen Cleary is a good name to trust on the topic (he has some blog articles on it). The .NET framework gives you a handful of ways to do multithreading, but Task/async/await is arguably the best implementation of it.

2. Understand that there is one main UI thread in an application, and ALL visual changes have to be handled by that UI thread. So if you want to update some visual element from a different thread, you have to use Invoke. It's not difficult - it's just sort of one of those weird "programmy" words that is simple to implement once you see it used.

3. And finally, data binding is your friend. Getting into the habit of using data models with properties (not fields) and using the INotifyPropertyChanged interface will make UI updates even easier. You've already got your feet wet here since you do data binding on the dropdown, which is a great first step.

When you get the above 3 concepts into your "toolbelt", you'll wonder how you ever got by without them.
Robb HillSenior .Net Full Stack Developer

Author

Commented:
Thank you!

Do more with

Expert Office
Submit tech questions to Ask the Experts™ at any time to receive solutions, advice, and new ideas from leading industry professionals.

Start 7-Day Free Trial