4
\$\begingroup\$
#region GetBookListByPriority
private static List<BCBook> GetBookListByPriority(List<BCBook> listBcBook)
{
    List<BCBook> newList = new List<BCBook>();
    try
    {
        List<BCBook> listNonPriorityBcBooks = new List<BCBook>();
        List<BCBook> listPriorityBcBooks = new List<BCBook>();

        foreach (BCBook bcBook in listBcBook)
        {
            BookHistoryDto historyDto = new BookHistoryFacade().FindHistoryByBookIdByPriority(bcBook.id.ToString(CultureInfo.CurrentCulture));
            if (historyDto != null && historyDto.HistoryId > 0)
            {
                bcBook.Priority = historyDto.Priority;
                listPriorityBcBooks.Add(bcBook);
            }
            else
            {
                listNonPriorityBcBooks.Add(bcBook);
            }
        }
        int count = 0;
        int prio = 0;
        foreach (BCBook bcBook in listNonPriorityBcBooks)
        {
            for (int j = count + 1; j <= 5; j++)
            {
                List<BCBook> listBook = new List<BCBook>();
                listBook = ListBookPriority(listPriorityBcBooks, j, newList);
                count++;
                if (listBook.Count() > 0)
                {
                    prio++;
                }
                else
                {
                    break;
                }
                foreach (BCBook bc in listBook)
                {
                    newList.Add(bc);
                }
            }
            newList.Add(bcBook);
        }
    }
    catch (Exception ex)
    {
        ErrorLogger.WriteErrorToLog(ex);
    }

    return newList;
}
#endregion

#region ListBookPriority
private static List<BCBook> ListBookPriority(List<BCBook> list, int priority, List<BCBook> newList)
{

    List<BCBook> listBook = new List<BCBook>();
    try
    {
        foreach (BCBook vid in list)
        {
            if (!IsExists(newList, vid.id.ToString(CultureInfo.CurrentCulture)))
            {
                if (vid.Priority == priority)
                {
                    listBook.Add(vid);
                }
            }
        }
    }
    catch (Exception ex)
    {
        ErrorLogger.WriteErrorToLog(ex);
    }
    return listBook;
} 
#endregion
\$\endgroup\$
  • \$\begingroup\$ You have several loops there. Which one is "this loop?" \$\endgroup\$ – Anthony Pegram Jul 6 '11 at 21:26
  • \$\begingroup\$ Step 1 is to add comments to your code explaining the intent. There is probably a better approach to what you are trying to do. Keeping track of count and prior seem odd since you never use either one. \$\endgroup\$ – jimreed Jul 7 '11 at 13:26
2
\$\begingroup\$

I would get rid of foreach and use for. I have found (and read about) a huge performance difference between the 2 (EDIT, but not always, see below).

Example (untested code):

int c  = listBcBook.Count;

for (int i = 0; i < c; i++)
{
    BCBook bcBook = listBcBook[i];

    BookHistoryDto historyDto = new BookHistoryFacade().FindHistoryByBookIdByPriority(bcBook.id.ToString(CultureInfo.CurrentCulture));
    if (historyDto != null && historyDto.HistoryId > 0)
    {
        bcBook.Priority = historyDto.Priority;
        listPriorityBcBooks.Add(bcBook);
    }
    else
    {
        listNonPriorityBcBooks.Add(bcBook);
    }
}

EDIT: I just read that the performance difference isn't always there because in some cases, the compiler will optimize your foreach loops into for loops. Here is a good read over on SO: https://stackoverflow.com/questions/1124753/for-vs-foreach-loop-in-c.

In your case, because you're using a List, directly using for should be faster.

\$\endgroup\$
2
\$\begingroup\$

While I don't have time at the moment to look through the code at the moment (and due to the lack of comments) I can't give a broad answer but I can already note two things.

Initialize your facade outside of the for loop.

BookHistoryFacade() facade = new BookHistoryFacade();
foreach (BCBook bcBook in listBcBook)
{
    BookHistoryDto historyDto =
       facade.FindHistoryByBookIdByPriority(bcBook.id.ToString(CultureInfo.CurrentCulture));
    ...
}

Use LINQ for your queries, which will also allow you to easily attempt Parallel LINQ. This would give you the opportunity to use several cores for your query ... aka as increased speed.

\$\endgroup\$
0
\$\begingroup\$

You can replace two foreach loops in GetBookListByPriority where you're adding the whole sublist to a new list with these:

newList.AddRange(listBook);
newList.AddRange(listNonPriorityBcBooks);
\$\endgroup\$
0
\$\begingroup\$

Its pretty unclear what the code does/needs to do. But I've tried to cut down on some of the code so try this (obviously untested) and see if its faster and does the same thing :)

    private static List<BCBook> GetBookListByPriority(List<BCBook> listBcBook)
    {
        List<BCBook> newList = new List<BCBook>();
        try
        {
            List<BCBook> listNonPriorityBcBooks = new List<BCBook>();
            List<BCBook> listPriorityBcBooks = new List<BCBook>();
            // move the variable historyDto outside the for loop so you create the object less.
            BookHistoryDto historyDto;
            foreach (BCBook bcBook in listBcBook)
            {
                historyDto = new BookHistoryFacade().FindHistoryByBookIdByPriority(bcBook.id.ToString(CultureInfo.CurrentCulture));
                if (historyDto != null && historyDto.HistoryId > 0)
                {
                    bcBook.Priority = historyDto.Priority;
                    listPriorityBcBooks.Add(bcBook);
                }
                else
                {
                    listNonPriorityBcBooks.Add(bcBook);
                }
            }

            int count = 1; 
            foreach (BCBook bcBook in listNonPriorityBcBooks)
            {
                // move the variable listBook outside the for loop so you create the object less.
                List<BCBook> listBook;
                // if you are only having a maximum of 5 then there is no need to create an extra variable, use the existing count
                for (count; count <= 5; count++)
                {
                    listBook = ListBookPriority(listPriorityBcBooks, count, newList);
                    // Count() is slower than Count, note the difference in the removal of the brackets
                    if (listBook.Count > 0)
                    {
                        // Only if the count is greater than 0 do we want to begin to enumerate the collection
                        foreach (BCBook bc in listBook)
                        {
                            newList.Add(bc);
                        }
                    }
                    else
                    {
                        break;
                    }
                }
                newList.Add(bcBook);
            }
        }
        catch (Exception ex)
        {
            ErrorLogger.WriteErrorToLog(ex);
        }

        return newList;
    } 

If you could comment your original code you would get a better response i'm sure

\$\endgroup\$
0
\$\begingroup\$

You are not optimizing anything with that:

int count = 1; 
// Some code here
for (count; count <= 5; count++)
{

This is equal in speed, but inferior in style to:

// Some code here
for (int count = 1; count <= 5; count++)
{

Avoid these "micro-optimizations". Even if done right, it is often the case that you will have much better things to improve.

\$\endgroup\$
0
\$\begingroup\$

If you don't want to use LINQ as Steven suggests, have you considered a SortedList? Just implement the IComparable interface on the priority object if it doesn't already.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.