tags:

views:

78

answers:

5

Hi chaps, can anyone recommend a nice and tidy way to achieve this:

float CalculateGoodness(const Thing& thing);

void SortThings(std::vector<Thing>& things)
{
    // sort 'things' on value returned from CalculateGoodness, without calling CalculateGoodness more than 'things.size()' times
}

Clearly I could use std::sort with a comparison function that calls CalculateGoodness, but then that will get called several times per Thing as it is compared to other elements, which is no good if CalculateGoodness is expensive. I could create another std::vector just to store the ratings and std::sort that, and rearrange things in the same way, but I can't see a tidy way of doing that. Any ideas?

Edit: Apologies, I should have said without modifying Thing, else it's a fairly easy problem to solve :)

+4  A: 

You can have a call to CalculateGoodness that you call for each element before sorting, and then CalculateGoodness simply updates an internal member variable. Then you can sort based on that member variable.

Another possibility if you can't modify your type, is storing some kind of std::map for your objects and their previously calculated values. Your sort function would use that map which acts as a cache.

Brian R. Bondy
+1 for caching the result. Exercise left to OP to set up the Thing class to cache the value of goodness, but also know when the object has changed and must be recalculated.
glowcoder
+1 as I used the exact same tip in my project today.
ereOn
Sorry, I should have added 'without modifying `Thing`'. Clearly the best answer if modifying `Thing` is allowed though, thanks.
Ben Hymers
+1  A: 

I'd create pairs of ratings and things, calling CalculateGoodness once per thing, and sort that on the rating. if applicable you could also move this to a map from rating to thing

the other option would be to cache CalculateGoodness in the Thing itself either as a simple field or by making CalculateGoodness a method of Thing (making sure the cache is mutable so const Things still works)

jk
+1  A: 

Perhaps a tidy way of doing the separate vector thing is to actually create a vector< pair<float, Thing*> >, where the second element points to the Thing object with the corresponding float value. If you sort this vector by the float values, you can iterate over it and read the Thing objects in the correct order, possibly playing them into another vector or list so they end up stored in order.

suszterpatt
Beware of using a pointer, it refers to the first vector which we will change while iterating afterward.
Matthieu M.
+3  A: 

I can think of a simple transformation (well two) to get what you want. You could use std::transform with suitable predicates.

  • std::vector<Thing> to std::vector< std::pair<Result,Thing> >
  • sort the second vector (works because a pair is sorted by it first member)
  • reverse transformation

Tadaam :)

EDIT: Minimizing the number of copies

  • std::vector<Thing> to std::vector< std::pair<Result,Thing*> >
  • sort the second vector
  • transform back into a secondary vector (local)
  • swap the original and local vectors

This way you would only copy each Thing once. Notably remember that sort perform copies so it could be worth using.

And because I am feeling grant:

typedef std::pair<float, Thing*> cached_type;
typedef std::vector<cached_type> cached_vector;

struct Compute: std::unary_function< Thing, cached_type >
{
  cached_type operator()(Thing& t) const
  {
    return cached_type(CalculateGoodness(t), &t);
  }
};

struct Back: std::unary_function< cached_type, Thing >
{
  Thing operator()(cached_type t) const { return *t.second; }
};


void SortThings(std::vector<Thing>& things)
{
  // Reserve to only allocate once
  cached_vector cache; cache.reserve(things.size());

  // Compute Goodness once and for all
  std::transform(things.begin(), things.end(),
                 std::back_inserter(cache), Compute());

  // Sort
  std::sort(cache.begin(), cache.end());

  // We have references inside `things` so we can't modify it
  // while dereferencing...
  std::vector<Thing> local; local.reserve(things.size());

  // Back transformation
  std::transform(cache.begin(), cache.end(),
                 std::back_inserter(local), Back());

  // Put result in `things`
  swap(things, local);
}

Provided with the usual caveat emptor: off the top of my head, may kill kittens...

Matthieu M.
That's the kind of tidiness I was after, thanks! It would be nice to avoid all the copies from and to the input vector but in this case (and in general I'd imagine) it will be cheaper to copy `Thing` than to call `CalculateGoodness`, and of course the algorithmic complexity is better than the comparison function approach.
Ben Hymers
I've added a way to avoid copying too much, since `sort` relies on copying.
Matthieu M.
Nice, I didn't think of `swap`. My compiler has move semantics, which may make even that unnecessary. And it has lambdas so I can make this even more pretty :) I wish I could upvote this twice!
Ben Hymers
+2  A: 

I've upvoted Brian's answer because it clearly best answers what you're looking for. But another solution you should consider is just write it the easy way. Processors are getting more powerful every day. Make it correct and move on. You can profile it later to see if CalculateGoodness really is the bottleneck.

Kristo
The Xbox 360 processor isn't getting any more powerful ;) But I didn't mention that so have an upvote anyway :)I should also mention this isn't a micro-optimisation, this is reducing the algorithmic complexity from O(log N) to O(N). I've not profiled it but I get a slight stutter when this code is run so I know it's (part of) a bottleneck.
Ben Hymers