refactor: extract-method hygiene pass on 3 large methods (finding 8) - #33
Merged
Conversation
Image.__init__ was 379 lines doing several unrelated jobs. Extracted two self-contained pieces: - _infer_dtype(image, dtype): staticmethod, pure function -- resolves the constructor's dtype= argument (auto-detect smallest int type, inherit via dtype=True, validate an explicit dtype, or reject dtype=False). Zero risk: no self access at all. - _reshape_to_size(self, image, size): the size= constructor argument handling -- reshaping 1D/oddly-oriented 2D input into the right (height, width, planes) shape. Added comprehensive test coverage first (TestImageInferDtype, TestImageReshapeToSize in test_image_core.py -- 22 new tests, one per branch/combination), since this code had real coverage gaps: no test for dtype=False, no test for either of the two "color plane count mismatch" ValueError paths in the reshape logic. All 22 pass against the extracted code. That coverage made a follow-up safe: _reshape_to_size had a `self.colororder is not None` branch that's provably dead code -- self._colororder is set to None at the very start of __init__ and never touched before this point, so the branch can never run. Confirmed via the getter (`return self._colororder`, no other logic) and by grepping for any other write to self._colororder earlier in the method -- none. Removed it and the now-unused color_dict parameter it was the only consumer of. Verified: full suite 840 passed (818 baseline + 22 new tests), 15 skipped, no regressions -- including immediately after the dead-code removal, confirming it was a true no-op as expected.
The nested fixdims()/lenkey() closures captured zero outer variables -- pure functions of their own parameters -- the lowest-risk extraction of this whole hygiene pass. Pulled out as _lenkey (staticmethod) and _fixdims (classmethod, calls cls._lenkey), matching the _infer_dtype/_plane_stats staticmethod pattern already used elsewhere in this class. Minor bonus: no longer recreates a closure on every __getitem__ call. Also closed real coverage gaps found while verifying this: no test exercised a stepped slice (im[::2, ::3], the n // step arithmetic branch in _lenkey, as opposed to the span-1 shortcut), and neither of the two "invalid slice" ValueError paths (wrong number of slice elements, or a key type that's neither int/str/tuple/list) had any test at all. Full suite: 843 passed (840 + 3 new tests), 15 skipped, no regressions.
CameraBase.plot() had zero test coverage before this -- adding it
first, ahead of an upcoming extract-method refactor, so there's an
actual before/after baseline instead of "trust me it still works".
Asserts on artist counts (ax.collections/ax.lines) for shape="frustum",
shape="camera", and frame=True, not deep rendering correctness.
Found and fixed a real test-isolation bug while getting these to pass
in the full suite: plot()'s ax=None path reuses whatever matplotlib
considers the "current" 3D axes (spatialmath.base.graphics.axes_logic
docs: "checks for a match with the passed axes ax or the current
axes"). Without closing figures in setUp, a figure left open by an
unrelated, earlier test in the suite got silently reused, making the
artist-count assertions meaningless (40 collections instead of 3,
passed in isolation, failed in the full run). Added plt.close("all")
to both setUp and tearDown.
Full suite: 846 passed (843 + 3 new tests), 15 skipped, no
regressions, stable across repeated runs.
CameraBase.plot() mixed a matplotlib-version-compat shim, two independent icon-drawing paths (frustum vs camera+cylinder), and a pose-frame overlay in one 175-line method. Extracted the two self-contained icon-drawing paths as _plot_frustum and _plot_camera_icon; left the compat shim (try/finally around ax.add_collection3d) and the frame overlay in plot() itself, since those aren't independent sub-tasks. Preserved verbatim rather than "cleaned up": an unused local variable (`a = 3 # length of axis line segments` in the camera-icon branch, never referenced) -- out of scope for a behavior-preserving extraction. Verified behavior-preserving: the characterization tests added in the previous commit (which had zero coverage before this refactor started) show byte-identical artist counts before and after this extraction. Full suite: 846 passed, 15 skipped, no regressions.
The only existing overlay-style test (test_hist_sorted_overlay_raises) covers the error path only -- nothing exercised actual overlay rendering (color assignment, filled polygons) before this. Adding ahead of an upcoming extract-method refactor of plot(), same reasoning as the Camera.plot() characterization tests: real coverage before touching the code, not after. Full suite: 847 passed (846 + 1 new test), 15 skipped, no regressions.
Histogram.plot() was ~420 lines. Extracted the three low-closure- capture pieces: - _compute_plot_series(type, samescale): the type-dispatch block that computes y/maxy/ylabel1/ylabel2. Pure computation over self.h/ self.pdf/self.cf/self.cdf. The `if filled is None: filled = True` side effect that lived inside the "frequency" branch stays in plot() itself as a one-line follow-up, since it mutates a variable used well beyond this block. - _histogram_plane_stats(counts): was a nested plane_stats() closure, called from both the stack and overlay branches (real duplication removed, not just relocated). - _resolve_overlay_colors(n, colors): per-plane color assignment for style="overlay". Mutates self.colordict as a side effect when unset -- preserved exactly, documented in the new method's docstring rather than silently dropped. Deliberately NOT extracted (per the plan for this hygiene pass): the stack-loop body, the cursor sub-block (nested on_move() closure capturing ~8 outer locals), or splitting stack/overlay into top-level methods -- all have heavy closure capture where extraction would just relocate the long-parameter-list smell into explicit arguments. Verified behavior-preserving: the 5 plot-related tests (including the two added in the previous two commits specifically to cover this method before touching it) pass with byte-identical assertions before and after. Full suite: 847 passed, 15 skipped, no regressions, stable across repeated runs.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 4 high |
🟢 Metrics 40 complexity · 0 duplication
Metric Results Complexity 40 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Conservative, behavior-preserving extract-method refactor on the large methods flagged in the code review, plus real test coverage added ahead of each risky change rather than after.
Image.__init__(ImageCore.py, was 379 lines): extracted_infer_dtype(staticmethod, pure) and_reshape_to_size. Added 22 new tests (TestImageInferDtype,TestImageReshapeToSize) covering every branch/combination — real gaps found: no test fordtype=False, no test for either of the two "color plane count mismatch"ValueErrorpaths. That coverage made it safe to then remove a genuinely deadself.colororder is not Nonebranch (self._colororderis alwaysNoneat that point in construction — confirmed via the property getter and by checking nothing sets it earlier) and its now-unusedcolor_dictparameter.Image.__getitem__(ImageCore.py, was 180 lines): extracted thefixdims/lenkeynested closures — captured zero outer variables, lowest-risk extraction in this PR. Added 3 tests closing real gaps: a stepped-slice case (im[::2, ::3], exercises then // steparithmetic branch) and both "invalid slice"ValueErrorpaths had zero coverage before.Camera.plot(Camera.py, was 175 lines): had zero test coverage at all before this PR. Added characterization tests first (artist-count assertions forshape="frustum",shape="camera",frame=True), then extracted_plot_frustum/_plot_camera_icon. Found and fixed a real test-isolation bug while getting these to pass in the full suite:plot()'sax=Nonepath reuses whatever matplotlib considers the "current" 3D axes, so a figure left open by an unrelated earlier test was silently reused, making artist counts meaningless (40 instead of 3) — fixed withplt.close("all")insetUp/tearDown.ImageProcessing.threshold: no extraction, per plan — the "196 lines" is mostly docstring; the real executable body is ~86 lines and already linear/well-scoped.Histogram.plot(ImageWholeFeatures.py, was ~420 lines): onlystyle="overlay"'s error path had a test before this PR — added a happy-path test (style="overlay", filled=Trueon a 3-plane histogram) first, then extracted_compute_plot_series,_histogram_plane_stats(removes real duplication — was a nested closure called from both the stack and overlay branches), and_resolve_overlay_colors(mutatesself.colordictas a side effect — preserved exactly, documented rather than silently dropped). Deliberately did not extract the stack-loop body or the cursor sub-block (nestedon_move()closure capturing ~8 outer locals) — extraction there would just relocate the long-parameter-list smell into explicit arguments, not fix anything.Test plan