1
public Form1()
{
    InitializeComponent();
    loadComboEmail();
}

private void loadComboEmail()
{
    string path = Directory.GetCurrentDirectory();
    string build = (path + "\\" + "email.txt");
    string[] lines = System.IO.File.ReadAllLines(build);

    comboEmail.Items.AddRange(lines);
    comboEmail.SelectedIndex=0;
}

I've got a combobox that I wanted to load with the client email addresses which are stored in an text file. Using the loadComboEmail() I read from the text file and load the Combobox. Am I doing something that is considered bad practice by calling the loadComboEmail at the form startup? If so, how do I properly read from the textfile and then load it into the combo, so when the form loads, it has the necessary data?

cramopy
  • 3,459
  • 6
  • 28
  • 42

3 Answers3

2

No, this seems quite legit. You don't have to worry as this is the way all things get loaded in WinForms... This will block the UI thread for a short amount of time, but since your not going to load huge masses of something, you won't even notice it.

When you start performing a greater amount of Actions and big files you should think about using a BackgroundWorker or a Task!

Anyway you should also consider using the following instead of your Code:

private void loadComboEmail()
{
    string path = System.IO.Path.GetDirectoryName(Application.ExecutablePath); //seems to be the same to me, but is safer
    string build = System.IO.Path.Combine(path, "email.txt"); //much more safer then simple string addition
    string[] lines = System.IO.File.ReadAllLines(build);

    comboEmail.Items.AddRange(lines);
    comboEmail.SelectedIndex = 0;
}
cramopy
  • 3,459
  • 6
  • 28
  • 42
  • @Nexusfactor no problem, I'm glad I was able to help you. Please mark my answer as solved, if i helped you :D – cramopy Jun 07 '15 at 13:38
  • I was just wondering, threading is a concept that is taking me awhile to wrap my head around. Seeing as I'll just be reading from a text file, and loading some form controls, using async isn't nessesary in my case correct? Using the code that Martijn van Put gave me, it still says it going to run synchronously so it won't matter to use his correct? –  Jun 07 '15 at 13:39
  • @Nexusfactor That's true, as you're not loading let's say 200 MB of text and processing it. So you don't have to worry about and using it right now. – cramopy Jun 07 '15 at 13:41
  • Thanks, Much appreciated. –  Jun 07 '15 at 13:41
0

You can use:

 Task task = Task.Factory.StartNew(() =>
 {
    // Background work
    loadComboEmail();
 }).ContinueWith((t) => 
 {
    //If you need to execute something after the loading process.
 });

In order to update the UI thread, you can maybe do the reading on another thread and when the list of emails is loaded, just update it.

Task.Factory.StartNew(() =>
        {
            // Background work - read the file
        }).ContinueWith((t) => {
            // Update UI thread here

        }, TaskScheduler.FromCurrentSynchronizationContext());
Olaru Mircea
  • 2,570
  • 26
  • 49
0

Using the constructor to load that is not considered as a bad practice. Read the answer of Hans Passant about when you should use the Load Event of the window. What setup code should go in Form Constructors versus Form Load event?

Altough as said in comments, you are blocking the UI Thread. In the constructor the keyword await cannot be used. So you have to 'Fire and forget'. When using the Load event you can use await but event handlers are async void. So they are not awaited, you have stil 'Fire and forget'.

public Form1()
{
    InitializeComponent();
    loadComboEmail();
}

private async Task loadComboEmail()
{
    string path = Directory.GetCurrentDirectory();
    string build = (path + "\\" + "email.txt");
    string[] lines = await Task.Run(() => File.ReadAllLines(build));

    comboEmail.Items.AddRange(lines);
    comboEmail.SelectedIndex=0;
}
Community
  • 1
  • 1
Martijn van Put
  • 3,293
  • 18
  • 17
  • 1
    It wil run but it give this warning: Warning 3 This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread. C:\Users\Nexusfactor\documents\visual studio 2013\Projects\Crystal Message\Crystal Message\Form1.cs 30 28 Crystal Message –  Jun 07 '15 at 13:36
  • Correct, some incorrect coding. Please try again with the new sample. You now still get a warning because awaiting could not be done inside the constructor. Other solution is to use a continuation task as shown by Olaru Mircea – Martijn van Put Jun 07 '15 at 13:45
  • @ Martijn van Put - I tried it. It works now. Do you have an article or somewhere you learned all this (task, await) I would like to read and understand on my own. –  Jun 07 '15 at 13:50
  • Nice it works now! Yes ofcourse, take a look here: https://msdn.microsoft.com/en-us/library/hh191443.aspx. Also o'Reilly has a nice book about Async: http://shop.oreilly.com/product/0636920026532.do – Martijn van Put Jun 07 '15 at 13:51
  • 1
    Quick question, I notice sometimes, it loads but the combobox isn't loaded, I have to close and re-open the application again. Any reason why? –  Jun 07 '15 at 14:09
  • Put a try catch inside the loadcomboemail could be anything, maybe the file is in use? – Martijn van Put Jun 07 '15 at 14:13
  • No, I don't have it open. It doesn't throw an error. It just doesn't load, I have to close it, and reload it again from Visual Studio. –  Jun 07 '15 at 14:15
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/79895/discussion-between-martijn-van-put-and-nexusfactor). – Martijn van Put Jun 07 '15 at 14:15