[Discussion] Where should `isDate1904` and `getDateTimeFormat` reside? (`DateUtils` vs. `AbstractDateTimeConverter`)
- Dominant language
- Java
- Stars
- 6.2k
- Forks
- 532
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 42
Description
#### Background & Current State
In several recent PRs introducing or improving date/time converters (e.g., `SqlTime*`, `Year*`, `OffsetDateTime*`, `SqlDate*`, etc.), we noticed that there is some duplicated boilerplate code, along with inconsistencies in edge-case handling:
**1. Inconsistent format extraction and empty string handling**
- Some implementations check `StringUtils.isEmpty(format)` and normalize it to `null`, while others pass empty strings directly downstream, which can lead to unexpected parsing errors.
- The format extraction logic is repeatedly implemented across different converters:
```java
String format = null;
if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) {
format = contentProperty.getDateTimeFormatProperty().getFormat();
}
```
**2. Layering issue with 1904 windowing check**
Currently, `DateUtils` contains the following method:
```java
public static boolean isDate1904(ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration)
```
This causes the low-level, general-purpose date utility class to directly depend on framework-level metadata models (`ExcelContentProperty` and `GlobalConfiguration`), introducing architectural coupling.
---
#### Proposed Options
To standardize the implementation for future date/time types, eliminate boilerplate code, and unify edge-case behaviors, I suggest we tidy up this bit of logic. Two main directions are being considered:
##### Option A: Centralize into `DateUtils` (Keep the status quo)
Add `getDateTimeFormat(ExcelContentProperty)` to `DateUtils`, keeping it at the same level as the existing `isDate1904`.
- **Pros:** Minimal changes required; no new class hierarchy introduced; keeps the codebase flat.
- **Cons:** The low-level utility class continues to take on framework-level metadata resolution, causing its responsibilities to bloat further.
##### Option B: Introduce an abstract base class `AbstractDateTimeConverter`
- Encapsulate `getDateFormat(ExcelContentProperty)` and `isDate1904(ExcelContentProperty, GlobalConfiguration)` within the base class.
- ~Since `DateUtils.isDate1904` was introduced recently and **has not yet been included in any official release**, it can be directly deprecated or removed without backwards-compatibility baggage.~ (already in the 2.1.0 release candidate)
- Concrete converter subclasses can focus solely on core data conversion logic.
- **Pros:**
- Unifies edge-case behaviors.
- Keeps metadata resolution visible only to date/time converters, preserving the purity and single responsibility of `DateUtils`.
- **Cons:** Introduces a new abstraction layer and inheritance hierarchy.
---
#### Feedback Welcome
Which option do you prefer? Or do you have alternative suggestions? Feel free to share your thoughts below!
- `+1 [Option A / Option B] {additional information - optional}`
- `-1 [Option A / Option B] {reason}`
- `+0 [Alternative proposals or thoughts]`
Contributor guide
Research direction
Start by reading DateUtils and the date/time converter implementations mentioned in the discussion, including SqlTime*, Year*, OffsetDateTime*, and SqlDate*. Compare the duplicated format extraction and isDate1904 handling against Options A and B. Done means reaching agreement on where these responsibilities should reside; the issue does not yet define an implementation or acceptance test.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100