Reduce code duplication
- Dominant language
- JavaScript
- Stars
- 15.7k
- Forks
- 3.9k
- Avg merge
- 4d 6h
- Merged PRs (30d)
- 34
Description
There is a considerable amount of duplication in the codebase. Until now, this was rather a subjective, anecdotal observation. I occasionally copied out two code blocks and switched back and forth between them, to check whether they are in fact equal, or whether there is s subtle difference (similar to a [blink comparator](https://en.wikipedia.org/wiki/Blink_comparator)).
It is _important_ (and often pretty easy) to avoid certain forms of duplication. And there's a reason why the first point of the first, most fundamental grade of the 'Clean Code Developer' principles is [Don't Repeat Yourself](https://clean-code-developer.de/en/the-straight/red-degree/#elementor-toc__heading-anchor-1).
Ironically, this (obviously) has not been repeated often enough.
Don't repeat yourself.
Don't repeat yourself.
Don't repeat yourself.
Of course there is duplication - for example, with error checks, or dummy implementations for certain interface functions, or creating some JSON structure in different places. And we can argue about "how many lines have to be duplicated" vs. "how difficult it is to pull that code into a standalone function", on a case-by-case basis.
But I recently stumbled over several places ([#12714](https://github.com/CesiumGS/cesium/issues/12714), [#12805](https://github.com/CesiumGS/cesium/issues/12805)) where no discussion is necessary: There are parts of the code that are duplicated in a way or form that cannot be justified. I mean, not _at all_ - not even with the usual "I didn't have enough time to make it 'better'".
I hoped that I could use the magic of ✨AI ✨ and just ask https://deepwiki.com/CesiumGS/cesium "What are the largest functions in the codebase that are duplicated?", but how should a "language model" know that...
So quickly tried out other tools:
- https://github.com/kucherenko/jscpd
- https://github.com/danielstjules/jsinspect
- https://github.com/pmd/pmd
Note: I have **not** performed a detailed analysis or comparison of these! I just ran them, with the default configuration, and looked at the results.
**jscpd** detects several duplications, and ends with a summary table that suggests that a staggering >11000 lines in CesiumJS are copy-pasted. The tool doesn't offer too much configurability, and the duplications that it finds are often pretty small. In JavaScript, I wouldn't even count "5 duplicated lines" as an _actual_ duplication.
**jsinspect** also finds many duplications, The tool also doesn't offer much configurability, but from scrolling over the results, some of them may be worth a look. Some of them are definitely "false positives", but maybe it's possible to narrow that down a bit.
These tools (jscpd and jsinspect) are JavaScript-based and JavaScript-focussed. They might already provide useful results, but also reported a lot of noise. For example, jsinspect reported duplications that appear to be purely _syntactical_, including comments (!). For example, it flags "dummy implementations of interfaces". This could be a hint at some deeper problem: Maybe there should be an abstract base class for that? More time might have to go into the analysis of the results here.
Both jscpd and jsinspect did **not** detect the blatant, 50-line, _verbatim_ copy-and-paste of the `getBinaryProperties` function, which is kind of disappointing. They did detect duplications in the `DataSource/...Batch...` classes, but seemed to struggle with that, and reported them as dozens of duplications with 10...20 lines each.
**pmd** (is a Java❤️-based tool that) allows some important configurations - for example, whether `foo=42` and `foo=43` should be considered to be "equal", or whether different variable names (in _structurally_ equal code) should be considered to be "equal". I did not thoroughly investigate the effects of these options. But with a run in the default configuration, it did detect the `getBinaryProperties` and `DataSources/...Batch...` duplications cleanly. If I had to start somewhere, I'd start with that.
---
All tools will generate what one **could** call "false positives". An overly specific example: The `CorridorGraphics` and `WallGraphics` constructors both contain the block
```
this._cornerTypeSubscription = undefined;
this._granularity = undefined;
this._granularitySubscription = undefined;
this._fill = undefined;
this._fillSubscription = undefined;
this._material = undefined;
this._materialSubscription = undefined;
this._outline = undefined;
this._outlineSubscription = undefined;
this._outlineColor = undefined;
this._outlineColorSubscription = undefined;
this._outlineWidth = undefined;
this._outlineWidthSubscription = undefined;
this._shadows = undefined;
this._shadowsSubscription = undefined;
this._distanceDisplayCondition = undefined;
this._distanceDisplayConditionSubscription = undefined;
this._classificationType = undefined;
```
Now, that's not _real_ duplication, right? It's a good practice to initialize member variables after all. But I think that this might still be a strong hint that these classes could be considered for a refactoring - maybe by pulling these things into some `BaseGraphics` class.
The amount of work is required for avoiding certain _forms_ of duplication varies wildly, depending on deep, structural and architectural aspects of the underlying software. But there are some _really_ low-hanging fruits in CesiumJS, and we should pick them as soon as possible.
Contributor guide
Research direction
Start by running PMD or reviewing its reported duplicates, especially the getBinaryProperties function and the DataSources/...Batch... classes. Compare the tool results with the examples involving CorridorGraphics and WallGraphics, then determine which duplication is actionable. Done means agreeing on a focused refactoring scope and removing justified duplication without treating false positives as defects.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- tooling
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100