Skip to content

Conversation

@ppkarwasz
Copy link
Contributor

The current factory pattern used with the AbstractManager registry is unnecessarily complex. It relies on:

  • Stateless factory classes to create managers of each type,
  • Separate FactoryData parameter objects to pass construction data

What this PR does

This PR simplifies the registry factory mechanism by:

  • Replacing the factory classes with inline lambda factories
  • Allowing stateful factory lambdas, removing boilerplate FactoryData classes in most cases
  • Keeping factory logic localized and easier to follow within the registry call site

This is a pure refactoring, it does not change behavior. It improves code readability and maintainability by reducing indirection and eliminating unnecessary types.

Notes

FactoryData parameter objects are still used where necessary:

  • RollingFileManager and RollingRandomAccessFileManager still require parameter objects since they update state on each retrieval, not just during initialization.
  • Other managers still pass Configuration through ConfigurationFactoryData to access getLoggerContext(), which remains useful.

Checklist

🧪 Tests (select one)

  • I have added or updated tests to cover my changes.
  • No additional tests are needed for this change.

📝 Changelog (select one)

  • I added a changelog entry in src/changelog/.2.x.x. (See Changelog Entry File Guide).
  • This is a trivial change and does not require a changelog entry.

The current factory pattern used with the `AbstractManager` registry is unnecessarily complex. It relies on:

* **Stateless factory classes** to create managers of each type,
* **Separate `FactoryData` parameter objects** to pass construction data

#### What this PR does

This PR simplifies the registry factory mechanism by:

* Replacing the factory classes with **inline lambda factories**
* Allowing **stateful factory lambdas**, removing boilerplate `FactoryData` classes in most cases
* Keeping factory logic **localized and easier to follow** within the registry call site

This is a **pure refactoring**, it does not change behavior. It improves code readability and maintainability by reducing indirection and eliminating unnecessary types.

#### Notes

`FactoryData` parameter objects are still used where necessary:

* **`RollingFileManager`** and **`RollingRandomAccessFileManager`** still require parameter objects since they update state on each retrieval, not just during initialization.
* Other managers still pass `Configuration` through `ConfigurationFactoryData` to access `getLoggerContext()`, which remains useful.
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

I agree that most FactoryData implementations are unnecessary boilerplate – but I'm not chilled to see ManagerFactory implementations get inlined. Ideally getFileManager() should pass a DTO instead of 3 dozen arguments, but that is a thicker refactoring.

@ppkarwasz ppkarwasz enabled auto-merge (squash) November 11, 2025 20:58
@ppkarwasz ppkarwasz merged commit 4b7065b into 2.x Nov 11, 2025
7 checks passed
@ppkarwasz ppkarwasz deleted the feat/refactor-manager-factories branch November 11, 2025 21:12
@ppkarwasz
Copy link
Contributor Author

Ideally getFileManager() should pass a DTO instead of 3 dozen arguments, but that is a thicker refactoring.

This is exacly what I tried to eliminate here: a private DTO (FactoryData) that is used in a single method is more confusing than useful. Configuration data ends up being packed and unpacked multiple times:

  1. It is packed into a FileAppender.Builder,
  2. It is unpacked and passed to FileManager.getFileManager(),
  3. It is packed into a FileManager.FactoryData,
  4. It is unpacked again by FileManagerFactory,
  5. It is finally used be the FileManager constructor.

All along this path, the data is validated multiple times, defaults are applied and also some side-effects (creation of parent directory) appear.

If FileManager was a class carefully designed to be reusable, then I agree: a DTO (possibly a Java record) as only parameter to getFileManager() would be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants