================================================================================ SKYCHART (Cartes du Ciel) - CODE IMPROVEMENT SUGGESTIONS ================================================================================ This file covers structural, performance and maintainability improvements. Actual defects are in the companion file skychart-bugs.txt; a few items here cross-reference it. Nothing below is a correctness problem. These are changes that would make the code easier to work on, faster, or less likely to grow new bugs. They are ordered roughly by payoff-to-effort ratio. Measurements were taken on the supplied source: 105 units, 128,689 lines, plus the 63 .lfm form files and the .lpi/.lpr project files from the second archive (the .pas files are byte-identical between the two archives). ================================================================================ I1. Break up the three oversized units ================================================================================ pu_main.pas 14,352 lines (501 KB) cu_skychart.pas 9,303 lines fu_chart.pas 8,195 lines cu_catalog.pas 6,762 lines cu_plot.pas 5,958 lines cu_planet.pas 4,599 lines pu_main.pas alone is 11% of the codebase. It contains the main form, the menu and toolbar construction, the configuration reader and writer, the TCP command dispatcher, the DSS/image download logic, the language switcher and the Windows-specific registry probing. It also holds 161 `with` statements, which is where most of the "which object does this field belong to?" ambiguity in the project lives. Suggested split for pu_main.pas, in decreasing order of ease: u_configfile.pas LoadDefault/SaveDefault/SaveChartConfig/ SavePrivateConfig/SaveQuickSearch and their readers (~1,500 lines, almost no form dependencies) u_toolbarsetup.pas InitToolBar and the image-list plumbing u_cmddispatch.pas the TCP command table and its handlers pu_main.pas the form itself The configuration unit is the best first move: it is self-contained, it is where several of the swallowed-exception bugs live (see bugs B1), and extracting it makes the config format testable without a GUI. Recommended practice going forward: treat 2,000 lines as the point at which a unit needs splitting, and add that to the contributing notes. ================================================================================ I2. Deduplicate the two ASCOM mount drivers ================================================================================ cu_ascommount.pas 1,295 lines cu_ascomrestmount.pas 1,170 lines line-level similarity: 54% The two units implement the same T_mount interface over two transports: COM automation (`V.tracking`) and the Alpaca REST API (`V.Get('tracking').AsBool`). Everything above the transport layer - unit conversion, coordinate handling, state machine, timer logic, rate-table parsing, error messages - is duplicated. Compare, for example: cu_ascommount.pas:769-777 cu_ascomrestmount.pas:737-743 if not VarIsEmpty(V) then if FStatus<>devConnected then exit; try try result:=V.tracking; result:=V.Get('tracking').AsBool; except except end; end; This is roughly 600 lines of duplicated logic. Every fix has to be applied twice - and the empty-handler pattern shows the copies have already drifted (cu_ascommount checks VarIsEmpty, cu_ascomrestmount checks FStatus). Suggested structure: T_ascombase = class(T_mount) // all shared logic protected function GetProp(const name: string): Variant; virtual; abstract; procedure SetProp(const name: string; value: Variant); virtual; abstract; function CallMethod(const name: string; args: array of Variant): Variant; virtual; abstract; end; T_ascommount = class(T_ascombase) // ~150 lines: COM transport T_ascomrestmount = class(T_ascombase) // ~150 lines: REST transport The same pattern applies, to a lesser degree, to cu_ascomrest.pas and cu_alpacamanagement.pas. ================================================================================ I3. Extract the repeated astronomical formulas into shared helpers ================================================================================ The same law-of-cosines elongation/phase computation appears in TPlanet.Comet (cu_planet.pas:2587-2595) and TPlanet.Asteroid (cu_planet.pas:2721-2722). The comet copy clamps the arccos argument; the asteroid copy does not - which is bug A4. That is exactly what duplicated formulas cost. Suggested helpers, in u_projection.pas or a new u_astromath.pas: function SafeArcCos(x: double): double; inline; begin if x >= 1 then Result := 0 else if x <= -1 then Result := pi else Result := arccos(x); end; procedure ElongationPhase(r, dist, sunDist: double; out elong, phase: double); There are 53 arccos/arcsin call sites in the project. Several already carry ad-hoc domain fudges that show the author ran into the problem repeatedly: u_projection.pas:415 r := (arccos(s1 * s2 + c1 * c2 * c3 - 1e-12)); u_projection.pas:563 de := (arcsin(s2*s3 - c2*c3*c1)) + 1E-9; u_projection.pas:677 de := (arcsin(...)) + 1E-9; Subtracting 1e-12 from the argument is not a clamp - it fails for arguments above 1 + 1e-12 and it biases every result slightly. A single SafeArcCos / SafeArcSin pair replaces all of these with something correct and self- documenting. The same applies to the airmass-to-altitude block, which is copy-pasted verbatim at pu_obslist.pas:242-255 and pu_obslist.pas:806-818. ================================================================================ I4. Rewrite words() - it is quadratic and on the hot path ================================================================================ File: u_util.pas, lines 801-826 function words(str, sep: string; p, n: integer; isep: char = blank): string; begin Result := ''; str := trim(str); for i := 1 to p - 1 do begin j := pos(isep, str); if j = 0 then j := length(str) + 1; str := trim(copy(str, j + 1, length(str))); <-- copies the remainder end; for i := 1 to n do begin j := pos(isep, str); if j = 0 then j := length(str) + 1; Result := Result + trim(copy(str, 1, j - 1)) + sep; str := trim(copy(str, j + 1, length(str))); <-- again end; end; Three problems: (a) PERFORMANCE. Each field skipped copies and re-trims the entire remaining string. Extracting field p from a line of length L costs O(p*L) with a heap allocation per step. This function is called per field, per line, during catalogue parsing, FITS header parsing (cu_fits.pas), MPC element import (cu_database.pas) and horizon loading. Parsing an 80-column FITS header card for field 9 does nine full-string copies. An index-walking version that never copies the input is roughly an order of magnitude faster and allocates once: function words(const str: string; const sep: string; p, n: integer; isep: char = blank): string; var i, start, stop, len: integer; begin Result := ''; len := Length(str); i := 1; // skip p-1 fields ... end; (b) TRAILING SEPARATOR. `Result := Result + trim(...) + sep;` appends sep after the last field too. `words(s, ', ', 1, 3)` returns "a, b, c, ". Every caller that passes a non-empty sep has to trim the result again. (c) NO WAY TO DETECT A MISSING FIELD. A field that does not exist and a field that is empty both return ''. Callers then do `StrToFloat(trim(words(...)))`, which throws - and, in most of the file loaders, lands in an empty except block. A `TryWords(...): boolean` variant, or returning the field count via an out parameter, would let the loaders report "line 47: missing declination" instead of silently producing a broken catalogue. Given how central this function is, it is worth a small unit test file (see I9). ================================================================================ I5. Replace the 82 interface-level globals in u_constant.pas ================================================================================ u_constant.pas has a 2,034-line interface section declaring 82 global variables, alongside the type and constant definitions the unit is named for. Almost every other unit uses u_constant, so those globals are effectively ambient state for the whole program. This is what makes the codebase hard to reason about: any procedure anywhere can read or write appdir, ConfigDir, VerboseMsg, NightVision, the star colour tables, the catalogue lists, and so on. Incremental path that does not require a rewrite: 1. Split the file: u_types.pas (types and true constants) and u_appstate.pas (the globals). This alone makes the dependency visible in every uses clause. 2. Group related globals into a singleton record or class (TAppPaths, TDisplaySettings, TCatalogState). 3. Convert the ones that are only written once at startup into read-only properties of that singleton. Step 1 costs an afternoon and pays for itself the first time someone needs to know who mutates a given flag. ================================================================================ I6. Reduce the reliance on `with` ================================================================================ pu_main.pas 161 `with` statements u_util.pas 33 pu_calendar.pas 33 cu_plot.pas 27 fu_config_display.pas 25 `with` in Object Pascal silently resolves an identifier against the with-object first, then the enclosing scope. Adding a field to the with-object can change the meaning of existing code without any compiler diagnostic. With 161 of them in a 14,000-line unit, a single new property on a form or config class can reroute references anywhere in the file. Nested `with` compounds this. For example cu_tcpserver.pas:269 has `with Fsock do begin ... end` wrapping 45 lines that also touch `cmd`, `cmdresult`, `active_chart` and `terminated`. Recommendation: do not remove them all at once, but adopt "no new `with`" as a rule, and unwind them opportunistically in any block being modified for another reason. Short local aliases read just as well: s := Fsock; s.MaxLineLength := 1024; ================================================================================ I7. Make failures visible ================================================================================ See bugs B1 for the full list of 77 empty handlers. The improvement, as opposed to the bug fix, is infrastructure: - The project already has `WriteTrace` and a `VerboseMsg` flag. Route every handler through it. - Add a small helper so the common case is one line and impossible to get wrong: procedure LogSwallowed(const context: string; E: Exception); begin WriteTrace(context + ': ' + E.ClassName + ': ' + E.Message); end; used as: except on E: Exception do LogSwallowed('LoadCometFile', E); end; - For the configuration writers specifically (pu_main.pas SaveDefault, SaveChartConfig, SavePrivateConfig, SaveQuickSearch), return a boolean and surface a message box on failure. Losing a user's chart configuration silently is the worst failure mode in the program. - Turn hints back on. cdc.lpi sets `` in every build mode that specifies it (debug, debug heap, windows, win64, cocoa, gtk2, qt6), so the compiler's own diagnostics are suppressed project-wide. At least two findings in this review would have surfaced as hints on a normal build: bug A10 destructor with no `inherited Destroy` (u_CacheBMP.pas:87, u_orbits.pas:988) bug B4 self-assignment `h := h;` (u_projection.pas:1357, 1408) Hints were presumably disabled because the existing build produces too many to read. The way out is to fix or explicitly suppress them once - with targeted `{$WARN nnnn OFF}` where a hint is genuinely unwanted - and then keep the build clean with -vwnh. A noisy diagnostic channel that everyone ignores is worth no more than a silent one. - While in the project options, align the runtime-check settings. The `release` and `windows` build modes have range, overflow, I/O and stack checking all off; the other six have them all on (see bugs B5 for the table). Whichever profile is intended, having two different ones means a fault that traps cleanly in one build silently corrupts state in another. ================================================================================ I8. Turn u_290.pas into a class ================================================================================ See bugs B2 and B3. The star reader is currently a set of free procedures over unit-level globals (`area290`, `buf2`, `record_size`, `maxmag`, `thefile_stars`, `Reader_stars`, `dec9_storage`, `database2`, `naam2`) plus an initialization section that wires five pointers at `@buf2[1]`. T290Reader = class private FBuffer: array[1..MaxRecordSize] of byte; FStream: TFileStream; FReader: TReader; FArea, FRecordSize: integer; ... public constructor Create(const CatalogPath: string); destructor Destroy; override; procedure ResetIndex; function Next(SearchMode: char; RA, Dec, FOV: double; out Star: TStarRecord): boolean; end; Benefits beyond tidiness: - Catalogue reading becomes threadable, which is the single biggest available win for chart redraw responsiveness on large fields. - The "you must call reset290index first" precondition becomes a constructor. - The buffer is sized from a constant, which closes bug A3 structurally rather than with a bounds check bolted on. - Two charts can read different regions at once. The public surface is small (two procedures), so this is a contained change. ================================================================================ I9. Add a test harness for the pure functions ================================================================================ The project has no tests. It also has a large body of code that is pure, deterministic and trivially testable without a GUI: u_util.pas words, wordspace, nospace, pos2, Str3ToAR, Str3ToDE, ARToStr*, DEToStr*, TimToStr, StrToTim, isodate, LeapYear, DayofYear, iso2date, datejd, jddate, decode_mpc_date, encode_mpc_date, Invert* u_projection.pas AngularDistance, Eq2Hz/Hz2Eq round-trips, precession, nutation, refraction (methods 1 and 2 are inverses) cu_planet.pas Kepler, PrecessElem, RectToPol u_290.pas the designation encoder (bug A2 would have been caught by a single assertion) FPCUnit ships with Free Pascal and needs no extra dependency. A single test project with fifty assertions would cover every formatting and parsing function in u_util.pas. Particularly high-value test cases, because they encode known-good answers: - Refraction: Refraction(h, true, ...) followed by Refraction(h, false, ...) should return the original altitude to within the documented tolerance. The code already carries reversibility correction constants (u_projection.pas:1411, `h := h - c.RefractionOffset;`) that nothing verifies. - Str3ToAR / ARToStr3 round-trip over the full range including negative declinations and the 23h59m59s boundary. - AngularDistance against Meeus worked examples, including the identical- point case (bug A5). - Kepler for e = 0, 0.5, 0.9, 0.97, 0.999 against published values. ================================================================================ I10. Smaller, localised improvements ================================================================================ a) cu_planet.pas Kepler solver (line 2389) The Newton iteration `c := (m + e*sin(E1) - E1) / (1.0 - e*cos(E1))` has a denominator that approaches zero as e -> 1 and E1 -> 0. The 10,000-iteration guard catches non-convergence but produces an exception message rather than a graceful degradation, and NaN never satisfies `ABS(c) < precision` so it always burns the full 10,000 iterations first. Add an explicit NaN/denorm check and bail out early: den := 1.0 - e * cos(E1); if abs(den) < 1e-12 then break; // fall through to the parabolic solver b) u_CacheBMP.pas - `Add` does `p.BMP := TBGRABitmap.Create; p.BMP.Assign(ABMP);` - a full pixel copy on every insertion. Since the caller immediately frees its own bitmap (cu_plot.pas:2299-2300), ownership transfer would avoid the copy entirely. - `Search` returns an index that `Delete` invalidates. Key the public API on the ID string instead of the index, so stale indices become impossible (see bugs B9). - The constructor and destructor are declared before the first visibility specifier, which puts them in the default section. Add an explicit `public`. c) u_util.pas ExecProcess / u_orbits.pas xplanet call Both (u_util.pas:3840-3870, u_orbits.pas:670-700) do: try p.Execute; except end; r.LoadFromStream(p.Output); irc := p.ExitStatus; If Execute throws, p.Output has not been populated and ExitStatus is meaningless, but the code carries on regardless. Move the post-processing inside the try, or check a success flag. d) cu_tcpserver.pas ThrdTerminate signature procedure ThrdTerminate(var i: integer); Takes an input-only id by reference, and is invoked as `FTerminate(id)` passing the thread's own field by reference. Change to a value parameter. e) pu_obslist.pas The airmass-to-altitude block at lines 242-255 and 806-818 is identical. Extract to `function AirmassToAltitude(am: double): double;`. f) Naming consistency The codebase mixes conventions: `Tf_chart`, `TSplot`, `Tskychart`, `T_ascommount`, `TCDCdb`, `TOrbits`, `hnskyhdr290_11`. Field prefixes vary too (`FStatus` vs `cfgsc` vs `area290`). A short style note in the contributing documentation - even just "new types are TPascalCase, new private fields are FPascalCase" - would stop further drift without requiring any renaming of existing code. g) Remove commented-out code cu_tcpserver.pas:239-242, cu_plot.pas:2247, u_orbits.pas:697, and numerous spots in pu_main.pas and fu_chart.pas carry disabled code with no explanation. Git already remembers it. h) Check whether -CF64 defeats the `extended` declarations (worth a look, not a confirmed problem) Every build mode in cdc.lpi passes `-CF64` in CustomOptions. If that is constraining floating point to 64-bit precision, then the deliberate use of `extended` in the projection code is not buying the extra headroom it looks like it is buying - for example: u_projection.pas:992 s1, s2, c1, c2, c3: extended; (AngularDistance) cu_planet.pas:2545 sOma, cOma, soe, coe, si, ci: extended; Either the `extended` declarations are load-bearing, in which case -CF64 needs a comment explaining why it is safe, or they are not, in which case they should become `double` so the intent matches the build. Whoever added -CF64 (most likely for cross-platform reproducibility, since x86_64 SSE and x87 differ here) will know which. Five minutes to settle, and it is the kind of thing that silently changes behaviour when someone edits the build file years later. i) Delete the two orphaned component declarations fu_config_chart.pas:228 (`Panel1: TPanel`) and fu_config_system.pas:81 (`Language: TTabSheet`) are published fields with no matching object in the .lfm and no Create anywhere. See bugs A14. Two one-line deletions. j) Keep the .lfm / .pas cross-check in CI The four consistency checks run for this review came back almost entirely clean - no missing event handlers, no duplicate component names, no duplicate keyboard shortcuts, and all 25 TTimer instances consistently Enabled = False at design time - which says the forms are well maintained. The one class of defect that did turn up (item i above) is invisible to the compiler, so it is worth a ~50-line script in the build pipeline: for each .lfm, parse the object tree and the matching form class, then report published component fields with no streamed object, OnXxx handlers with no matching method, and duplicate names or shortcuts. Cheap to write, and it catches exactly the mistakes that renaming or deleting a control leaves behind. ================================================================================ SUGGESTED ORDER OF WORK ================================================================================ Quick wins (hours, no structural risk): I3 SafeArcCos/SafeArcSin helpers - also fixes bug A4 and A5 I10a Kepler denominator guard I10e AirmassToAltitude extraction I10d ThrdTerminate signature I10g Delete commented-out code I10i Delete the two orphaned component declarations (bug A14) I7 (project options part only) align runtime checks across build modes and re-enable ShowHints - an edit to cdc.lpi, no source changes Medium (days, contained): I4 Rewrite words() + tests - biggest single perf win I7 Logging in place of empty handlers - biggest single debuggability win I9 FPCUnit harness for u_util and u_projection I8 T290Reader class - also closes bugs A3, B2, B3 I10j .lfm / .pas consistency check in CI I10h Settle the -CF64 vs `extended` question Larger (weeks, plan before starting): I1 Split pu_main.pas, starting with the configuration unit I2 Shared base class for the two ASCOM mount drivers I5 Split u_constant.pas and group the globals I6 Gradual retreat from `with` The two items I would do first are I4 and I7. words() is on every parsing hot path in the program, and the empty exception handlers are what will make every other investigation on this codebase slower than it needs to be.