views:

82

answers:

3

Currently, I have a static factory method like this:

public static Book Create(BookCode code) {
    if (code == BookCode.Harry) return new Book(BookResource.Harry);
    if (code == BookCode.Julian) return new Book(BookResource.Julian);
    // etc.
}

The reason I don't cache them in any way is because BookResource is sensitive to culture, which can change between the calls. The changes to culture needs to reflect in the returned book objects.

Doing those if-statements is possible a speed bottleneck. But what if we map book codes to anonymous functions/delegates? Something like the following:

delegate Book Create();
private static Dictionary<BookCode, Delegate> ctorsByCode = new Dictionary<BookCode, Delegate>();
// Set the following somewhere
// not working!
ctorsByCode[BookCode.Harry] = Create () => { return new Book(BookResource.Harry); }
// not working!
ctorsByCode[BookCode.Julian] = Create () => { return new Book(BookResource.Julian); } 
public static Book Create(BookCode code) {
    return (Book)ctorsByCode[code].DynamicInvoke(null);
}

How could I get those Create() => { lines to actually work?

Is this worth it speed-wise, when there are <50 book codes (thus <50 if-statements)?

This is a similar question, but unfortunately the author doesn't post his code http://stackoverflow.com/questions/713286/enum-delegate-dictionary-collection-where-delegate-points-to-an-overloaded-metho

Update

Did some performance benchmarks ifs vs delegates. I picked the unit code randomly and used the same seed for both methods. The delegate version is actually slightly slower. Those delegates are causing some kind of overhead. I used release builds for the runs.

5000000 iterations
CreateFromCodeIf ~ 9780ms
CreateFromCodeDelegate ~ 9990ms
+2  A: 

Like this:

private static readonly Dictionary<BookCode, Func<Book>> ctorsByCode = new Dictionary<BookCode, Func<Book>>();

...

ctorsByCode[BookCode.Harry] = () => new Book(BookResource.Harry);

public static Book Create(BookCode code) {
    return ctorsByCode[code]();
}

The Func<Book> type is a pre-defined generic delegate type that you can use instead of creating your own.
Also, your dictionary must contain specific a delegate type, not the base Delegate class.

Although lambda expressions are untyped expressions, they can be used in a place where a delegate type is expected (such as your indexer assignment) and will implicitly assume the type of the delegate.

You could also write

ctorsByCode[BookCode.Harry] = new Func<Book>(() => new Book(BookResource.Harry));

I would recommend changing your dictionary to contain BookResources instead, like this:

private static readonly Dictionary<BookCode, BookResource> codeResources = new Dictionary<BookCode, BookResource>();

...

codeResources[BookCode.Harry] = BookResource.Harry;

public static Book Create(BookCode code) {
    return new Book(codeResources[code]);
}
SLaks
Are you available for hiring? How much do you charge for an hour? :D Great solution. I like how it is a bit more compact than Keltex's. The alternative solution is actually very good.
randomguy
I'm getting "Delegate 'System.Action<Book>' does not take 0 arguments" with both of the above methods.
randomguy
@randomguy: Change it to `Func`; sorry.
SLaks
@randomguy: I forgot to mention that the `Dictionary` also ought to be declared `readonly`. (This declaration affects the variable, not the reference; it means you cannot write `codeResources = ...`.
SLaks
+2  A: 

Frankly you probably will not notice much a performance difference between the two. But if you're going to use the "functional" version, I would not just use a generic Delegate but instead pass the delegate that you just created. So this would be your (simpler) code:

delegate Book Create();
private static Dictionary<BookCode, Create> ctorsByCode 
    = new Dictionary<BookCode, Create>();

ctorsByCode[BookCode.Harry] = () => new Book(BookResource.Harry);
ctorsByCode[BookCode.Julian] = () => new Book(BookResource.Julian); 

public static Book Create(BookCode code) {
    return ctorsByCode[code]();
}
Keltex
Nice one, sir. Gonna accept SLaks answer, but gave you an upboat.
randomguy
A: 

randomguy, why are you complicating things so much? Two ways to avoid this mess in your question and in the answers that followed.

  1. BookResource bookResourceEnum = (BookResource)Enum.Parse(typeof(BookResource),bookCode);

    return new Book(bookResourceEnum);

  2. Or better yet, make your static factory method Create accept BookResource instead of the BookCode, and let clients worry about the conversion from code to enum so that nobody can pass in a code that cannot be mapped to BookResource.

Seriously, if you wrote me a code with 50 "if" or worse yet with 50 "delegates" we would have a "talk". Try to avoid premature optimization (if there is any) when you are not sure that is your bottle neck. Readability and maintainability should come first.

epitka
Hey! Thanks for the input. BookResource is a culture-sensitive resource class auto-generated from .resx file, thus it does not make sense to use it the way you suggest. Or at least it didn't make sense to me. I agree on all of the last points, however, I do feel the delegate method would be quite nifty.. if the overhead wasn't so big.
randomguy