Tech Off Thread

11 posts

Forum Read Only

This forum has been made read only by the site admins. No new threads or comments can be added.

Could this be done differently?

Back to Forum: Tech Off
  • User profile image
    tomtech999

    Ok, t he code below i wrote to build a library of songs, it should be fairly easy to follow. It scans 4 directories deep into a directory chosen my the user, and pulls out all the mp3 files. You will notice that at each file it runs a function called addtolibrary() (this function pulls out id3 info and sticks it into the database). My question is, is my code efficient, it often appears to have crashed when it hasn't and im sure it aint thread safe. It works perfectly but is ther a beter way of doing it? How would you accomplish the same thing??




    OdbcConnection librarywipe = new OdbcConnection("Driver={Microsoft Access Driver (*.mdb)};Dbq=" + Environment.CurrentDirectory + "\\library.mq;");
                librarywipe.Open();
                OdbcCommand go = new OdbcCommand("delete from data where artist like '%%'", librarywipe);
                go.ExecuteNonQuery();


                if (Directory.Exists(sourcepath.Text) == true)
                {
                    status.Visible = true;
                    logtext.Visible = true;
                    thelog.Visible = true;
                    thelog.Text = "";


                    int foldercount = 0;
                    int filecount = 0;

                    DirectoryInfo root = new DirectoryInfo(sourcepath.Text);
                    DirectoryInfo[] folder1 = root.GetDirectories();
                    add("Scanning started... " + DateTime.Now.ToShortTimeString());
                    foldercount += folder1.Length;
                    fol.Text = foldercount.ToString();

                    FileInfo[] files1 = root.GetFiles("*.mp3");

                    filecount += files1.Length;
                    fil.Text = filecount.ToString();
                    loadbar.Value = 20;


                    OdbcConnection library = new OdbcConnection("Driver={Microsoft Access Driver (*.mdb)};Dbq=" + Environment.CurrentDirectory + "\\library.mq;");
                    library.Open();
                    OdbcCommand drop = new OdbcCommand("delete * from data", library);
                    drop.ExecuteNonQuery();
                    library.Close();

                    //for each file in the root//
                    for (int i = 0; i < files1.Length; i++)
                    {

                        addtolibrary(files1[i]);

                    }

                    for (int j = 0; j < folder1.Length; j++)
                    {
                        //find files 1 level deep//
                        DirectoryInfo temp = folder1[j];
                        FileInfo[] files2 = temp.GetFiles("*.mp3");
                        filecount += files2.Length;
                        fil.Text = filecount.ToString();

                        for (int k = 0; k < files2.Length; k++)
                        {
                            addtolibrary(files2[k]);
                        }

                        DirectoryInfo[] temp2 = folder1[j].GetDirectories();

                        foldercount += temp2.Length;
                        fol.Text = foldercount.ToString();

                        for (int l = 0; l < temp2.Length; l++)
                        {
                            FileInfo[] files3 = temp2[l].GetFiles("*.mp3");
                            filecount += files3.Length;
                            fil.Text = filecount.ToString();

                            for (int m = 0; m < files3.Length; m++)
                            {
                                addtolibrary(files3[m]);
                            }

                            loadbar.Value = 60;

                            DirectoryInfo[] temp3 = temp2[l].GetDirectories();
                            foldercount += temp3.Length;
                            fol.Text = foldercount.ToString();

                            for (int n = 0; n < temp3.Length; n++)
                            {
                                DirectoryInfo temp4 = temp3[n];
                                FileInfo[] files4 = temp4.GetFiles("*.mp3");
                                filecount += files4.Length;
                                fil.Text = filecount.ToString();

                                for (int o = 0; o < files4.Length; o++)
                                {
                                    addtolibrary(files4[o]);
                                }
                                loadbar.Value = 40;

                            }

                        }

                        loadbar.Value = 100;
                    }



                    library.Close();
                    Done.Visible = true;
                    loadbar.Value = 100;
                    loadbar.Style = ProgressBarStyle.Blocks;
                    player.PlayMp3InThread(Environment.CurrentDirectory + "\\beep.mp3");
                    add("Scanning Complete... " + DateTime.Now.ToShortTimeString());
                    label7.Text = "Finsihed! " + dropcount.ToString() + " files were dropped. Library is ready, click next to continue.";
                    build.Text = "Rebuild";

    Thanks in advance!

    Tom

  • User profile image
    littleguru

    Just a few ideas Smiley I hope you don't mind...

    1) Use OleDbConnection and OleDbCommand
    2) Split it up into more methods (this method is to long!)
    3)  if (Directory.Exists(sourcepath.Text) == true) <= true is not required. Remove it.
    4) Split it up into different classes. You seem to interact with the UI and do business logic + database logic in one method. Not a good way to go!
    5) Create for example a class with name Mp3File, that holds a single mp3 file.
    6) Use try catch blocks to catch errors!
    7) http://channel9.msdn.com/ShowPost.aspx?PostID=209981 Read the book mentioned here. Smiley Covers quite everything.
    8) Use foreach instead of for(...) - for is more C, C++ style. foreach is C# (.NET) style.
    9) Don't nest for loops, try to split the whole thing up in different methods. Think also about splitting it up into different classes. I'm mentioning this twice, because you can do that really good here and it is very very important. .NET is all about objects (and OOP) and not about procedural programming.

    As a final thought: Try to do it a little bit more layered...

    - Database layer (just interacts with the DB)
    - Business layer (interacts with the DB layer and holds the objects and services that load, the files and get the information. This is the place for the Mp3File class and for example a DirectorySearcher class)
    - UI layer (interacts with the Business layer - NOT the database layer!! Sets the progress bar and labels. This is the place where the form and winform controls sit)

  • User profile image
    figuerres

    also lookup the info on "Backgound worker process"

    and with a bit of work you can do folder scans and db updates in the backgound and let the user do other stuff....


    basic is you add the compoent to a form and only do some control from the form ( stop, start, pause , show progress)
    then let the DoWork methood look about like this in general:


    while ( true)
    {
         if ( Not Halting )
    {
         Sleep(1 second)

         do stuff....

         report progress(  class with info)

        }
       else
          break;


    }


    the report progress method gets the "class with info"
    and does things like update the status bar, progress bar etc....

    all the work is done by a class "do stuff"
    that should do one thing and exit.

    then you can stop at the end of any one step and leave the system whole.

    and the user gets a multi threaded app that plays well with the OS.

  • User profile image
    tomtech999

    Thanks a lot guys this has really helped. I did attempted splitting this onto a seperate thread but had real problems with interaction with the controls. Anyway thanks for the constructive criticism , I don't mind at all!

    Tom:D

  • User profile image
    littleguru

    tomtech999 wrote:
    Thanks a lot guys this has really helped. I did attempted splitting this onto a seperate thread but had real problems with interaction with the controls. Anyway thanks for the constructive criticism , I don't mind at all!

    Tom


    You can't access the UI controls from another thread then the UI thread. That has been introduced with .NET 2.0. The reason is that otherwise the second thread could look the whole UI, which shouldn't happen! You can avoid that problem, but using a call back (delegate). You should google msn windows live search (man is this a long word, not very handy) about it...

  • User profile image
    figuerres

    littleguru wrote:
    
    tomtech999 wrote: Thanks a lot guys this has really helped. I did attempted splitting this onto a seperate thread but had real problems with interaction with the controls. Anyway thanks for the constructive criticism , I don't mind at all!

    Tom


    You can't access the UI controls from another thread then the UI thread. That has been introduced with .NET 2.0. The reason is that otherwise the second thread could look the whole UI, which shouldn't happen! You can avoid that problem, but using a call back (delegate). You should google msn windows live search (man is this a long word, not very handy) about it...


    actualy I belive (wish I had the reference to point to!) that win32 and GUI apps have this as a core "NO NO"
    not just .net

    as I recall (this may not be 100% but close)
    it has to do with the winforms and the windows "Message pump"
    ever notice that by default a gui app is marked at "sta"
    which means "Single Threaded Apartment" ??
    it's there for a reason....

    so in C / C++ / VB6 etc... you NEVER want to have 2 theads acting on a window.
    and all winforms controls are based on a window - controls in .net are windows in a window.

    WPF may alter this as I hear it's controls are not based on a win32 window.

    so a "Problem intereacting with the controls" is that 2 threads should not share control of a control.

    this is why in .net 2.0 they added the Background worker control.
    to help programers create a new thread not attached to the forms thread to run things in the background.

    they use events to allow one thread to pass data to another thread which can then act on this data.

    ERR -- ok BGWP is not a "Windowed" control it's just a bit of code that attaches to a form ? get it?

  • User profile image
    littleguru

    In .NET 1.0 and 1.1 you were able to edit a label or any other control from any other thread than the UI thread. I'm sure about that, because I did it and had to change that behaviour with .NET 2.0

  • User profile image
    Ang3lFir3

    I don't have a lot of suggestions as Littleguru covered most of my thoughts...

    especially breaking the code up....

    there are some things i would suggest using in a more detailed manner...

    1) generics
    2) collections instead of arrays
    3) Utilize a single instantiated(sp?) connection object and command object if applicable
    4) close connections ASAP... do everything you can to close them as fast as possible
    5) use SQL2K5Express and drop Jet all together... It's evil I tell you
    6) "delete from data where artist like '%%'" should just be "delete from data" the like test slows it down needlessly...btw why are you deleting everything in "data" twice or am i missing something?
    7) after "go.ExecuteNonQuery();" "librarywipe.Close()".... nonQuery() doesn't close the connection so its just hanging out... Connections are rare and valuable... give them back ASAP
    8) use more descriptive varNames

    thaz it off the top of my head....

  • User profile image
    littleguru

    Ang3lFir3 wrote:
    ... close the connection ...


    He could do the using pattern...

    using(OleDbConnection c = new OleDbConnection("connctionstring"))
    {
           // Add code here that requires the connection.
    }

    This ensures you that the connection is closed immediately after leaving the using block... I have put the WaitCursorScope project in the sandbox that demonstartes the using pattern:

    http://channel9.msdn.com/ShowPost.aspx?PostID=209184#209184

    The idea behind it is the same. As long as you are in the using block, the wait cursor is shown, immediately after leaving the block the cursor is set back.

  • User profile image
    ZippyV

    littleguru wrote:
    In .NET 1.0 and 1.1 you were able to edit a label or any other control from any other thread than the UI thread. I'm sure about that, because I did it and had to change that behaviour with .NET 2.0


    It was possible but discouraged multiple times on this forum, even by Larry O.

  • User profile image
    littleguru

    ZippyV wrote:
    
    littleguru wrote: In .NET 1.0 and 1.1 you were able to edit a label or any other control from any other thread than the UI thread. I'm sure about that, because I did it and had to change that behaviour with .NET 2.0


    It was possible but discouraged multiple times on this forum, even by Larry O.


    I know. It was bad practise... I have posted earlier here why you shouldn't do it...

Conversation locked

This conversation has been locked by the site admins. No new comments can be made.