add tm1637 7-segment LED display - #1677
Conversation
trim.1D3B2A9E-5839-4F0A-8DAC-D4E91ADAFFB2.MOV |
phoddie
left a comment
phoddie
left a comment
There was a problem hiding this comment.
Overall this looks really good and follows the ECMA-419 conventions well.
I added some comments for your consideration.
| if (typeof string !== "string") | ||
| string = String(string); |
There was a problem hiding this comment.
The typeof check isn't necessary - faster and smaller to unconditionally call String()
| } | ||
|
|
||
| #stopScroll() { | ||
| if (this.#timer) { |
There was a problem hiding this comment.
It is safe to call Timer.clear() on undefined, so this if is unnecessary
| } | ||
| } | ||
| catch (e) { | ||
| this.#onError?.(e); |
There was a problem hiding this comment.
technically this is not ECMA-419 style becauseonError() can be invoked from the public write() API. You can just throw here, since write() throwing is normal error behavior. But from the scrolling callback, you cannot throw since there is no one to catch, so your only options is to call onError(). You could do that in your timer callback, by catching errors thrown by #render() and rerouting them to #onError().
| const digits = options.digits ?? 4; | ||
| if (!Number.isInteger(digits) || (digits < 1) || (digits > MAX_DIGITS)) |
There was a problem hiding this comment.
JavaScript builtins that accept a number for an integer generally round the number rather than throwing if there is a fractional part. Maybe change to const digits = Math.round(options.digits) ?? 4 instead.
| ...ioOptions, | ||
| pin: clock, | ||
| mode: Digital.OutputOpenDrain, | ||
| initialValue: 1 |
There was a problem hiding this comment.
Nice use of new initialValue property!
| if (undefined !== options.rate) | ||
| this.#rate = Math.max(50, options.rate | 0); | ||
| if (undefined !== options.direction) | ||
| this.#direction = (options.direction === -1) ? -1 : 1; | ||
| if (undefined !== options.brightness) | ||
| this.#brightness = Math.max(0, Math.min(7, options.brightness | 0)); |
There was a problem hiding this comment.
If you move this down further in constructor, you might just be able to do this.configure(options); though... arguably these should not be options to the constructor, and the app should call configure() separately. (This guide hints at that, but as an implementation guide, rather than an API design guide, it doesn't go into details).
|
Thank you for the review. I had AI check whether the implementation followed the Guide, but it seems to have missed it. As for I have addressed the points you pointed out, so could you please check them? |
2 participants
I confirmed with Seed 4-Digit Display.