-1
\$\begingroup\$

I have in-house template like QList which creates a list of all objects of, say, Book.

This list contains many objects of Book. Book has member variable called BookTitle and in some cases objects have same value for BookTitle.

Now I am thinking of writing a function which contains objects of Book having unique book title. My function is working fine but I am wondering if I can improve it or maybe use a lambda for it. Can someone please shed light on it?

void createUniqueList{
unique_book_collection->deleteAll(); // deleteAll is in-house function 

Book* bookfirst = bookRecord->collection.at(0);
unique_book_collection->addRecord( bookfirst); // addRecord func in-house 

for( Book* l_book : bookRecord->collection())
{

         bool found = false;
        if(unique_book_collection != nullptr )
        {
        for( Book* l_uniqueBookRecord: unique_book_collection->collection() )
        {
            if( _uniqueBookRecord->bookTitle->description() == l_book->bookTitle->description() )
            {
                found = true;
                break;
            }
        }
        }
        if( found == false )
        {
            unique_book_collection->addRecord( l_book );
        }
    }

}
\$\endgroup\$

closed as off-topic by Edward, forsvarir, Jamal Oct 21 '16 at 13:28

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. The example code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Unlike Stack Overflow, Code Review needs to look at concrete code in a real context. Please see Why is hypothetical example code off-topic for CR? \$\endgroup\$ – Edward Oct 21 '16 at 12:59
1
\$\begingroup\$

Your collections seem to implement the standard container interface correctly since they work with range-based fors. This means that you can use standard functions from <algorithm>. Keeping your algorithm the same, your function becomes:

void createUniqueList() {
    unique_book_collection->deleteAll();

    using std::begin;
    using std::end;
    auto &col = unique_book_collection->collection();

    for(Book *b : bookRecord->collection())
        if(std::find_if(begin(col), end(col), [&](Book const *b2) {
            return b2->bookTitle->description() == b->bookTitle->description();
        }) == end(col))
            unique_book_collection->addRecord(b);
}

Notes:

  • if(unique_book_collection != nullptr) has been removed. You dereferenced it earlier in the function and didn't change it in the meantime, so it cannot be null.
  • The first book has no special treatment : it won't be found in the empty list, and will be added as any other.
\$\endgroup\$

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