[Task]: Error prone issues & Code Clarity Bugs

[stankiewicz]

What needs to happen?

Error Prone & Code Clarity Bugs

1. Duplicate Code Branches
   File:
   srcs/sdks/harness/src/main/java/org/apache/beam/fn/harness/FnApiDoFnRunner.java
   Line: 475
   Issue Type: DuplicateBranches
   Line Fragment:
   java

    mainInputConsumer = new SplittableFnDataReceiver() { ... };
    this.processContext = new WindowObservingProcessBundleContext();
  } else {
    mainInputConsumer = new SplittableFnDataReceiver() { ... };
    this.processContext = new WindowObservingProcessBundleContext();
  }

Suggested Fix: Both branches contain identical code. Remove the conditional check and the else block, keeping only a single implementation if the logic is truly identical.

2. Incorrect use of JodaTime Constructors
   File:
   srcs/sdks/harness/src/main/java/org/apache/beam/fn/harness/FnApiDoFnRunner.java
   Line: 1368
   Issue Type: JodaConstructors
   Line Fragment: fireTimestamp = new Instant(DateTimeUtils.currentTimeMillis());
   Suggested Fix: Use Instant.now() or Instant.ofEpochMilli(DateTimeUtils.currentTimeMillis()).
   Change: fireTimestamp = Instant.now();

3. Misleading Preconditions Check
   File:
   srcs/runners/java_fn_execution/src/main/java/org/apache/beam/runners/fnexecution/control/TimerReceiverFactory.java
   Line: 86
   Issue Type: PreconditionsCheckNotNullRepeated
   Line Fragment: checkNotNull(receivedElement, "Received null Timer from SDK harness: %s", receivedElement);
   Suggested Fix: Including the variable in the error message for a null check is redundant as it will always be null.
   Change: checkNotNull(receivedElement, "Received null Timer from SDK harness");

4. Format Strings ConcatenationFile: srcs/sdks/harness/src/main/java/org/apache/beam/fn/harness/FnApiDoFnRunner.java Lines: 2088, 2131, 2141, 2427, 2467, 2477, 2737, 2777, 2787
   Issue Type: FormatStringShouldUsePlaceholders
   Example Fragment (L2477): checkState(mainOutputSchemaCoder != null, "Output with tag " + mainOutputTag + " must have a schema...");
   Suggested Fix: Use message templates with placeholders instead of string concatenation.
   Change: checkState(mainOutputSchemaCoder != null, "Output with tag %s must have a schema...", mainOutputTag);

5. Test File Cleanup (JodaTime & Mocking)Multiple Files: FnApiDoFnRunnerTest.java, SimpleDoFnRunnerTest.java, StatefulDoFnRunnerTest.java, etc.
   Issue Type: JodaConstructors (Many instances)
   Fragment: new Instant(...), new Duration(...)
   Fix: Use Instant.ofEpochMilli(...) or Duration.millis(...).
   File: srcs/sdks/harness/src/test/java/org/apache/beam/fn/harness/control/ProcessBundleHandlerTest.java
   Lines 708, 737: DoNotMockAutoValue - BundleProcessor and InMemory are @autovalue classes and should not be mocked.
   Lines 401, 438: ImpossibleNullComparison - Comparing non-null values to null.

6. Code Clarity Improvements
   File: srcs/runners/java_fn_execution/src/test/java/org/apache/beam/runners/fnexecution/wire/CommonCoderTest.java (L155): AutoValueBoxedValues - Unnecessary boxing in AutoValue.
   File: srcs/runners/core/src/test/java/org/apache/beam/runners/core/SimplePushbackSideInputDoFnRunnerTest.java (L93): StronglyTypeTime - Use Duration instead of long for time values.
   File: srcs/runners/core/src/test/java/org/apache/beam/runners/core/SimplePushbackSideInputDoFnRunnerTest.java (L539): UnusedVariable - Remove unused parameter c.

Issue Priority

Priority: 3 (nice-to-have improvement)

Issue Components

• Component: Python SDK
• Component: Java SDK
• Component: Go SDK
• Component: Typescript SDK
• Component: IO connector
• Component: Beam YAML
• Component: Beam examples
• Component: Beam playground
• Component: Beam katas
• Component: Website
• Component: Infrastructure
• Component: Spark Runner
• Component: Flink Runner
• Component: Samza Runner
• Component: Twister2 Runner
• Component: Hazelcast Jet Runner
• Component: Google Cloud Dataflow Runner