search

andywilsonuk / StringTokenFormatter

Provides string extension methods for replacing tokens within strings (using the format '{name}') with their specified lookup value.
Apache License 2.0
37 stars 1 forks source link

Here are the changes! #15

Closed TonyValenti closed 5 years ago

TonyValenti commented 5 years ago

Hi @andywilsonuk - After doing a bit more profiling, I wasn't able to get the entirity of the performance gains I was hoping for. The compiled value retrieval is much faster than using reflection (and the compilation cost is negligible), however, after further delving I was able to determine that value retrieval was actually not a huge percentage of the operation - string manipulation and looping was.

There are three main changes in this PR:

  1. Use compiled expressions for value retrieval.
  2. Added "TokenToPrimitiveValueMapper.cs" as a way to short-circuit iterating all of the default value mappers (basically, if a property is a string/int/etc, we don't need to loop through all the value mappers).
  3. Refactored FormatProviderValueFormatter so that Format only calls into String.Format (which is slow!) if it is absolutely necessary.

These enhancements took my tests from taking ~1.9 seconds to run down to about 1.1 seconds.

andywilsonuk commented 5 years ago

Great, I'll check it out and get back to you.

andywilsonuk commented 5 years ago

Hi Tony, I've added a few comments for you to comment on but generally looks good.

TonyValenti commented 5 years ago

@andywilsonuk - Thanks Andy. I responded to your items in-line. Do you want me to change them or you? Also, what are your thoughts about the bigger changes I suggested in https://github.com/andywilsonuk/StringTokenFormatter/issues/16 ?

andywilsonuk commented 5 years ago

ok, I really like what you've done here around consistency and add more defaults for people to use (like in markers). My only comment is that you've got some inconsistency with parameter names, they should be camelCase but you've got some TitleCase. Considering the size of change I think that's pretty good!

TonyValenti commented 5 years ago

Sounds great! I'll review and get that changed. My preference is for title case. Can I use that or do you have a preference for camel?

andywilsonuk commented 5 years ago

Camel is c# standard so let's go with that.

TonyValenti commented 5 years ago

@andywilsonuk I got the changes made! Can you give me permission to merge this in? Also, I've never done a nuget release. Do you want to do that or should I?

TonyValenti commented 5 years ago

Are there any other changes you'd like to see?

andywilsonuk commented 5 years ago

Nope I think we are all good. I've added you as contributor now so you should be able to merge the changes. After that I'll create the new nuget package and upload it.

TonyValenti commented 5 years ago

@andywilsonuk - All merged in!

andywilsonuk commented 5 years ago

I've created the package and uploaded it to nuget.org, it'll hopefully be available soon.

Well done on a great upgrade!