apache / apache/fesod

[Discussion] Where should `isDate1904` and `getDateTimeFormat` reside? (`DateUtils` vs. `AbstractDateTimeConverter`)

Open
#1,097 2 comments 1 reaction 0 assignees View on GitHub
discussion welcome
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.