Version 2.0.0: Refactor Effort

[ryanelian]

I would like to discuss a possible future for this project.

I have studied the source code and discovered issues that may cause difficulty in future development or testing. Many classes are very tightly coupled with each other. For example:

• RedisStorage.Prefix is a static string that is instantiated from constructor but is being used everywhere for obtaining the Redis Keys.

• Source code is lacking XML documentation.

• RedisConnection is a god class / object, that is being passed around and used by multiple services.

• Services are tightly coupled into other services due to instance creation in-methods.

WIP / Phase 1

Therefore, I would like to address these concerns by championing the version 2.0.0 of the library. This is the preview1 of the refactor attempt: https://github.com/ryanelian/Hangfire.Redis.StackExchange/commit/c035c9bdd7e38f322e1a421a5a07b652d89650b2

Which covers the following efforts:

• Introduce Keychain service as a dependency to multiple classes for determining Redis key name, instead of using RedisStorage.Prefix.

• RedisStorageOptions is now the single source of truth for user-defined settings. This allows RedisStorageOptions to be used as a service to be injected into classes that require it as dependencies.

• Update NuGet packages (patch semver).

These refactors are categorized low impact and should not alter application logic but greatly improve the code structure.

All 83 tests still pass:

Test run for E:\VS\Hangfire.Redis.StackExchange\Hangfire.Redis.Tests\bin\Debug\netcoreapp1.1\Hangfire.Redis.Tests.dll(.NETCoreApp,Version=v1.1)
Microsoft (R) Test Execution Command Line Tool Version 15.0.0.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
[xUnit.net 00:00:00.5883651]   Discovering: Hangfire.Redis.Tests
[xUnit.net 00:00:00.7669704]   Discovered:  Hangfire.Redis.Tests
[xUnit.net 00:00:00.7732262]   Starting:    Hangfire.Redis.Tests
[xUnit.net 00:00:01.4850830]   Finished:    Hangfire.Redis.Tests

Total tests: 83. Passed: 83. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 1.9087 Seconds

Future Work / Phase 2

If you do not agree with above efforts, I will respect that decision and cease further work.

However if you're on board with the idea, I would like to proceed (during my spare times in the next 3-4 months) to the Phase 2 of the refactor, which covers these efforts:

• Turn magic strings passed to Keychain into methods that accepts parameter and produce relevant keys.

• Implement factory pattern as a service for instantiating new classes.

• Refactor logic various classes that operate on RedisConnection space.

• Try to solve #39

---

External API that used commonly by developers, such as of RedisStorageExtensions will not change. But internal API will be adjusted to accommodate the new architecture.

Let me know what you think. 🌶

[marcoCasamento]

Hi @ryanelian and thanks for your big effort. I do agree with your considerations, RedisConnection knows too much and the prefix thing has been made a bit in a hurry, out of a need.

I hope you understand that I need a bit of time in order to review the code and be sure to truly understand every single line of code and test the code in a pre-production environment within one of my customer, where Hangfire process about 250K jobs a day. Also, I have to make sure that everything works with Hangfire.Pro that I cannot put here due to license restriction.

Just one consideration about the versioning and features, I 'd also like to make sure that I meet expectations for Hangfire.Core 2.0 as described here: https://github.com/HangfireIO/Hangfire/issues/929. I'd like that Hangfire.Redis 2.0 supports Hangfire.Core 2.0, going otherwise I think could cause confusion among our users. Perhaps we can stick with ver 1.8.0 ?

[ryanelian]

I hope you understand that I need a bit of time in order to review the code and be sure to truly understand every single line of code and test the code in a pre-production environment within one of my customer, where Hangfire process about 250K jobs a day. Also, I have to make sure that everything works with Hangfire.Pro that I cannot put here due to license restriction.

Yes, please. After all, this is less of a "pull request" and more of bringing the ideas to the table.

In fact, I'm glad that you have a way of 'stress testing' the library. 👍

Just one consideration about the versioning and features, I 'd also like to make sure that I meet expectations for Hangfire.Core 2.0 as described here: HangfireIO/Hangfire#929. I'd like that Hangfire.Redis 2.0 supports Hangfire.Core 2.0, going otherwise I think could cause confusion among our users. Perhaps we can stick with ver 1.8.0 ?

Sounds good.

Either we can finish refactoring first before 2.0 released (I doubt it, seeing our lack of free time...) or refactor later after 2.0 has been released.