[I] [Task]: Error prone issues & Code Clarity Bugs [beam]

["stankiewicz (via GitHub)" <gi...@apache.org>]

stankiewicz opened a new issue, #37743:
URL: https://github.com/apache/beam/issues/37743

   ### 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
   ```if (doFnSignature.processElement().observesWindow() || ... ) {
       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
   - [x] 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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [Task]: Error prone issues & Code Clarity Bugs [beam]

["stankiewicz (via GitHub)" <gi...@apache.org>]

stankiewicz closed issue #37743: [Task]: Error prone issues & Code Clarity Bugs
URL: https://github.com/apache/beam/issues/37743


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org