Sudoku review #1
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "BordedDev/sudoku:main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Hey, I finally got round to creating a PR (creating a draft until I can add more notes)
@ -0,0 +18,4 @@
return false
}
class EventHandler {
A pretty standard, pretty robust way of handling events. I'd lean to the built-in EventTarget until the need for something better is needed.
@ -0,0 +237,4 @@
}
}
class Row extends EventHandler {
The Row/Col classes are confusing in the current situation, since Row isn't used for more than a bag holder/event forwarding
@ -0,0 +228,4 @@
}
}
toString() {
Seeing someone implement a custom
toString
is always nice@ -0,0 +287,4 @@
states = []
parsing = false
_initialized = false
initalized = false
Why are there 2 initialized properties?
Even worse, one is wrongly written :P
@ -0,0 +678,4 @@
-ms-user-select: none;
background-color: #e5e5e5;
border-radius: 5px;
aspect-ratio: 1/1;
Unfortunately, this doesn't work with dynamic inside out sizing/nested aspect-ration on firefox. Luckily, you can use
em
since the size is based on font sizeSo they're not nice squares on firefox? Upgrade to chrome!
Never! Google can claw my faux-privacy from my cold dead hands (also ublock). I've been considering using chromium/vavaldi so I can get functional HDR (right now I use edge when I need chrome)
@ -0,0 +680,4 @@
border-radius: 5px;
aspect-ratio: 1/1;
}
.sudoku-field-initial {
You can combine styles as well, I find it to be a bit easier to work with on average since you can toggle and combine them when it's needed much easier.
Also CSS now supports nested styling rules:
Ah facks. Thanks. Yh, my css skills are very outdated.
Still better than my junior. I was very surprised, learning about the CSS upgrade ~this~ last year
@ -0,0 +693,4 @@
color: red;
}
.sudoku-field {
border: 1px solid #ccc;
Not sure if the double border was intentional, I assume it wasn't.
The way I solved it was making the border on the bottom and right, you can use the nth selector to disable it on the edges if need be
@ -0,0 +767,4 @@
this.fieldElements = []
this.puzzle = new Puzzle(9)
this.fields = []
this.styleElement = document.createElement('style');
From what I've read, it's not recommended to create elements in the constructor and instead should be done in the
connectedCallback
I'm not sure about that. Will look it up, but you kinda want to have the component ready right when it get 'connected'? I also use that event somewhere I guess.
It's here: https://developer.mozilla.org/en-US/docs/Web/API/Web_components/Using_custom_elements
That's what the connected event is for. They break that "rule" in the example on the same page, and yeah, it gets called through the
_bind
function. I think the comment was originally meant to be on the_bind
call in the constructor@ -0,0 +13,4 @@
}
on(event, listener) {
this.events[event] ||= { name: name, listeners: [], callCount: 0 }
Simplified the empty event handler assignment. It was meant to be
??=
not||=
both will work in this case because of you how JS works though@ -0,0 +43,4 @@
if (this.suppresEvents) return [];
this.eventCount++;
const returnValue = this.events[event].listeners.map(listener => {
var returnValue = listener(data)
var :(
Oh no! A VAR!
@ -0,0 +348,4 @@
reset() {
this._initialized = false
this.initalized == false;
Accidental comparison 😅
Oh fuck, that can maybe be the reason something is not working or why even the second variable is implemented. Because I really don't know what the reason was.
@BordedDev nice review so far! I will check your new files tomorrow. I'm now fighting with speech recognition and command execution. It's a cool project and it was open source for a while but then i really, really start to rape the code. But check how easy it is to add functions to the LLM:
The docstrings are instructions for the LLM. I have somewhere a class inspector converting that nice to LLM commands.
So, if i need new funcitonality, I just add it there.
@ -0,0 +6,4 @@
There is a bunch of unused code
Autosolver is broken, I had to move the function around
Ah, sad. I didn't know status of the code. But if it's working again, i will put the sudoku on my site.
@ -0,0 +30,4 @@
*
* @type {(number|null|undefined)[]}
*/
#activeState = new Array(9 * 9)
Ooooh, magic numbers :P Did i have that too?
Dang it I missed it, it's meant to be
SUDOKU_GRID_SIZE
. You did with thenew Puzzle(9)
Nice review so far!
Interesting, I ended up creating a class with a
cmd_
prefix for functions the AI can call, but I have to add the function manually to the prompt, not that it would ever call anything otherimg
andreact
. For the message it matches[ img | Some image]
using a handwritten parser with the intent of moving it to Lark. How do you manage it or does it directly generate the python?^ That whole system is what I want to replace, which is why I was looking at TTS/STT since it's heavily Discord dependant with FastApi crowbarred in
@ -0,0 +91,4 @@
||
field.row.index == this.row.index && field.value == this._value)
}).filter(field => field != this).length == 0
A double filter, it's inefficient (since both will loop through the array) but for the size it doesn't really matter
@ -0,0 +295,4 @@
this.debugEvents = true;
this.initalized = false
this.rows = []
if (isNaN(arg)) {
Nice usage of the coercion, even if my IDE complains because there is coercion
@ -0,0 +391,4 @@
this._initialized = false;
const regex = /\d/g;
const matches = [...content.matchAll(regex)]
Regex strikes again 🏃💨. Technically inefficient, practically I like it.
BTW it's intentionally not an array it returns so that you can efficiently stream the results through the iterator (that's how I understand the below)
Hey @BordedDev come talk at https://rock.molodetz.nl.
@ -0,0 +1,73 @@
:host {
display: inline-block;
--border-radius: 5px;
I'd normally use a @property for this but it wasn't working the initial value so I did this instead
https://developer.mozilla.org/en-US/docs/Web/CSS/@property
@ -0,0 +3,4 @@
max = Math.floor(max)
return Math.floor(Math.random() * (max - min + 1)) + min
}
I ran Biome.js over this, and I'm a
;
hater (my default config), that's why they're not here@ -0,0 +248,4 @@
this.puzzle = puzzle
this.cols = []
this.index = this.puzzle.rows.length
const me = this
A lot of these
me
proxies aren't necessary with the arrow functions you're using, they're removed in the modified version@ -0,0 +300,4 @@
return invalid[invalid.length - 1]
}
for (const row of this.rows) {
Switched
.forEach
tofor of
loop for performance (recommended practice, but honestly, it’s not a big deal - I'd even call it a nitpick)@ -0,0 +365,4 @@
const matches = [...content.matchAll(regex)]
const max = this.size * this.size
for (const [index, match] of matches.entries()) {
Collapsed the index to come from the
matches
array, since I noted the 1-to-1 relation@ -0,0 +769,4 @@
} else {
const keyLookup = {
u() {
puzzle.popState()
This is mostly personal preference, since an "industry standard" way would be key => action enum => (action transform =>) action function. I find this a bit easier to read. If I'd bring in a library for the matching I'd have used
cond
from lodash https://lodash.com/docs/4.17.15#cond@ -0,0 +19,4 @@
.sudoku-field {
aspect-ratio: 1 / 1;
width: 1em;
line-height: 1;
This is to fix the vertical alignment since it had a value of 1.5 for me
@ -0,0 +13,4 @@
* @param min {number}
* @param max {number}
* @returns {number}
*/
Type hints are here since I can see them in my IDE and make things slightly easier to work with, hence why I've put them everywhere but with no actual docs
@ -0,0 +29,4 @@
*
* @type {SudokuState[]}
*/
#state = []
This contains a stack of states, essentially the initial state + all steps
@ -0,0 +43,4 @@
*/
get grid() {
const gridValue = [...this.#activeState]
Object.freeze(gridValue)
This mainly to enforce that the underlying grid isn't modifiable, technically we return a new array anyway, so there is little point.
#activeState
is also always replaced so we can freeze it when it's created, but this felt more flexible@ -0,0 +75,4 @@
for (const char of serializedState) {
if (PARSE_RANGE_START <= char && char <= PARSE_RANGE_END) {
if (VALID_RANGE_START <= char && char <= PARSE_RANGE_END) {
Since 0 is an invalid value for the
SudokuState
this allows for them to be filtered out while still increasing the number placement@ -0,0 +95,4 @@
* @returns {undefined|number}
*/
#sampleRandomEmptyField(stateToSample = this.#activeState) {
const iters = [...stateToSample.entries().filter(([, v]) => v == null)]
Recent learned that the variables are optional in destruction, had them as
_
before.@ -0,0 +126,4 @@
const left = Math.trunc(Math.trunc(slot % 9) / 3) * 3
const top = Math.trunc(Math.trunc(slot / 9) / 3) * (9 * 3)
for (let y = 0; y <= 27; y += 9) {
I'm being a little cheeky here, making the multiplication an addition instead. I can see an argument that this should be a 1 to 3 loop with multiplication, I liked the simplicity of
positionIndex
this way@ -0,0 +184,4 @@
* @param state {SudokuState}
* @return {Set<number>}
*/
getValues(slot, state = this.#activeState) {
These return a
Set
to simplify/combine the logic for automatically resolving and checking if a number is valid. In theory, the check would be faster since it can check early@ -0,0 +268,4 @@
}
class SudokuHost extends HTMLElement {
static observedAttributes = ["size", "puzzle", "readonly"]
These were necessary for me to have
attributeChangedCallback
work@ -0,0 +304,4 @@
#keyProcessors = new Map()
get isActive() {
return this.matches(":hover")
This was more convenient for me than the mouseenter/mouseexit events
@ -0,0 +425,4 @@
this.#styling = document.createElement("link")
this.#styling.rel = "stylesheet"
this.#styling.href = "sudoku.rewrite.css"
this.#root.appendChild(this.#styling)
This was more convenient for me than replacing the outer HTML with a
<style>...</style>
string to get syntax highlighting30062089f6
to77e78c8031
WIP: Sudoku reviewto Sudoku reviewCheckout
From your project repository, check out a new branch and test the changes.