Skip to content

Refactor favicon middleware, avoid magic numbers.#1919

Merged
ReneWerner87 merged 2 commits into
gofiber:masterfrom
vikpe:master
May 28, 2022
Merged

Refactor favicon middleware, avoid magic numbers.#1919
ReneWerner87 merged 2 commits into
gofiber:masterfrom
vikpe:master

Conversation

@vikpe

@vikpe vikpe commented May 27, 2022

Copy link
Copy Markdown
Contributor

Increase readability / follow best practices.

@welcome

welcome Bot commented May 27, 2022

Copy link
Copy Markdown

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

ReneWerner87
ReneWerner87 reviewed May 28, 2022
View reviewed changes

@ReneWerner87 ReneWerner87 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, what about the benchmark

Does this adjustment cost allocations?

@vikpe

vikpe commented May 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Update: I introduced a constant for the standard path and ran the benchmark.

master

cpu: Intel(R) Core(TM) i5-4670K CPU @ 3.40GHz
Benchmark_Middleware_Favicon
Benchmark_Middleware_Favicon-4          13465311                86.32 ns/op            3 B/op          1 allocs/op
Benchmark_Middleware_Favicon-4          13408497                87.49 ns/op            3 B/op          1 allocs/op
Benchmark_Middleware_Favicon-4          13480440                87.96 ns/op            3 B/op          1 allocs/op
Benchmark_Middleware_Favicon-4          12733093                87.11 ns/op            3 B/op          1 allocs/op

pr

cpu: Intel(R) Core(TM) i5-4670K CPU @ 3.40GHz
Benchmark_Middleware_Favicon
Benchmark_Middleware_Favicon-4          13345022                86.64 ns/op            3 B/op          1 allocs/op
Benchmark_Middleware_Favicon-4          13459353                86.65 ns/op            3 B/op          1 allocs/op
Benchmark_Middleware_Favicon-4          13541000                86.20 ns/op            3 B/op          1 allocs/op
Benchmark_Middleware_Favicon-4          13520474                86.09 ns/op            3 B/op          1 allocs/op

However I guess one need to run benchmarks on a dedicated setup (not my dev computer) for persistent results.

@ReneWerner87 ReneWerner87 merged commit 68e922d into gofiber:master May 28, 2022
@welcome

welcome Bot commented May 28, 2022

Copy link
Copy Markdown

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

trim21 pushed a commit to trim21/fiber that referenced this pull request Aug 15, 2022
@vikpe @trim21
Refactor favicon middleware, avoid magic numbers. (gofiber#1919)
b0d6f1a 
* Refactor favicon middleware, avoid magic numbers.

* Introduce constant for path to favicon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ReneWerner87 ReneWerner87 ReneWerner87 approved these changes

Assignees

No one assigned

Labels

🧹 Updates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vikpe @ReneWerner87