Fork me on GitHub
#re-frame
<
2024-06-02
>
bibiki16:06:26

hi. i'm practicing reframe so i wrote this small minesweeper game using it. here is the code https://github.com/bibiki/games/blob/main/src/cljs/games/pages/minesweeper.cljs it'd be nice if somebody with experience using reframe looked at it and gave any feedback. one thing i am in particular interested to know is this: i have a start button to initiate a new layout of mines. however, initially, i wanted the layout to be initiated immediately as the pages is accessed. for that purpose, i had a line that said (rf/dispatch [:gridalize]) I could see the relevant reg-event-db was running as i had a println inside it, but my grid of mines would still come null inside my game function where i subscribe to the value of grid. I could not figure out why that was, so I just added the start button to have more control over that. if somebody knows how I could have done that, I d be happy if you'd shared that with m e

p-himik16:06:24

Where was that dispatch line initially?

bibiki16:06:46

just above where the defnition of my game function is

p-himik16:06:45

Also, do you want feedback just about re-frame or also about CLJ/S or even programming in general?

bibiki16:06:58

yeah yeah, that too is welcome

p-himik16:06:34

Using dispatch at the top level of a namespace means that it will be called when the namespace is loaded - not when the desired URL is navigated to. in games.core, you have routes definitions with a home route that dispatches some init event - that will overwrite any DB state that exists at that point. A proper fix would be moving that gridalizing dispatch to the :start handler of the route. Might also make it a tad more generic and wrap the call to dispatch in some init function in relevant namespaces that are then called by the router. So you don't clump together routes and specific event IDs.

bibiki16:06:19

Im not sure I follow this "A proper fix would be moving that gridalizing dispatch to the :start handler of the route."

bibiki16:06:34

do you mean dispatch event inside (game function?

bibiki16:06:27

aha, like line 63 inside core.cljs

bibiki16:06:21

yeah, that did work. thanks @U2FRKM4TW. please feel invited to share more of your feedback regarding anything you see could be better.

p-himik17:06:37

Some other things: • Stuff like (and (<= 0 y) (> cols y)) can be written as (< -1 y cols) • I like how you organized directions inside neighbors. But it's unlikely to survive any formatting tool, so you might want to add a bunch of ,,, instead of the repeating whitespace in the center (a comma is treated as a regular whitespace by Clojure) • repeated (repeat 8 [x y]) - no need to hardcode that 8 in there, map stops on the shortest collection. I would also inline that usage, IMO it doesn't need a separate name • neighbors (map #(map + %1 %2) repeated directions) - here, you turn "point vectors" into "point lazy collections". Probably not a big deal at all here, but I'd still replace the inner map with mapv(-> grid (nth x) (nth y)) - this pattern, with various indentations/spellings/contexts , is repeated a few times, I'd move it into a separate get-cell function • is-bomb? returns a number. It would be better if it returned a boolean and the code that uses it would then (count (filter identity ...)) or something like that • Names like is-bomb? and :is-open? are an antipattern, I'm pretty sure. I think there's a note on it somewhere in the community-driven Clojure Style Guide (although I myself don't agree with everything in that guide). In any case, I myself would write it as just bomb? and open?(rf/reg-sub :grid ...) could be written shorter: (rf/reg-sub :grid :-> :grid). More details are in the docstring of rf/reg-sub • Simple events that do nothing but assoc'ing a thing into app-db might deserve their own shorthand. I would also suggest for any project that's not a quick test of something to copy around a wrapper of re-frame that you wrote yourself and can adapt to your own needs. Makes a lot of things smoother down the road. E.g. in my own wrapper I make it so that event IDs are never passed to event/sub handlers, so I don't have to write _ everywhere; I have more advanced shorthands for reg-sub than the ones built into re-frame, like that :->; and so on • Lines 105-106 - the formatting is confusing and makes the reader count the parens. I'd move lined to the next line and probably even that % as well. Also, since it's a conditional assoc, I'd replace the whole if in there with cond-> • In the cond inside click-handler you call dispatch in every branch. Maybe it's worth it to move that dispatch outside of cond and make the latter return the event vector • :key (str r "x" c) - you don't have to make strings yourself, if you want you can use :key [r c] or something like that • :width "20px" :height "20px" - when dealing with pixels in :style in Reagent, there's no need to specify the unit and use a string, you can simply say 20 instead of "20px". But some folks prefer when it's explicit • :border-style "solid" - similarly, "solid" can be :solid if you prefer that • (if (:is-open? cell) (:content cell)) - it's not a rare suggestion to replace single-branched ifs with whens • In games.core, you have r/with-let that's indented as a function. There's probably a way in your IDE to make it be indented as a regular let • Stuff like :section.section>div.container>div.content - of course YMMV and it might be a matter of preference, but after years of using that style I stopped and pretty much never use those shorthands anymore. It's easier to write but harder to refactor, to extend, to see nesting at a glance, to search for keywords

bibiki19:06:53

thanks a lot @U2FRKM4TW for taking the time to review

👍 1