Closed mtorpey closed 4 years ago
Hi Michael! Many thanks for having a look through the code. I am somewhat embarrassed that I left in the comment from line 539 in solver.js - I should have thought to look at this before pushing my code! Apologies for that.
Re: all other suggestions, these are very helpful and I appreciate your feedback. I'll go through and implement those changes today and should have the code pushed in an hour or two. Other than these fixes, I will probably take a couple days off for the weekend as I haven't been very good at taking breaks.
Once I've made the changes you've suggested above, I'll push the code to the repo again. If I don't hear from you before then, I will see you on Tuesday for our meeting!
Hi Cameron! I just had a look through your code in its current state (commit 981d75c) and had a few comments.
Firstly, this is still great progress! It looks like you've already done most of what we talked about in our last meeting, and it seems to be working great, modulo those bugs you've spotted. Well done so far!
Most of my comments here are stylistic things. Some are quite fussy, but it'd be great to have a nice clean codebase for further work and for when it's being marked. Here goes...
Some very long lines - some of your comments in particular are going off the edge of a typical screen. A good limit is 80-100 characters.
Spacing around syntax like
<=
,=
and;
are inconsistent. I favour spaces around operators, and one space after a semi-colon. e.g.rows=1;rows<=boardheight;rows++
should berows = 1; rows <= boardheight; rows++
. An automated formatter/prettifier might make this job quicker.changeBool
isn't very descriptive. How about something likefoundNewInfo
?It may be time to start cutting/improving some log messages. Try to identify messages you always ignore when debugging, and consider removing them.
A few instances of
for (i=...)
should probably befor (let i=...)
to keep local variables local.Lots of unnecessary comparisons to booleans. For example, you could change
item.isMine == false && item.guessMine == true
to!item.isMine && item.guessMine
unless you feel thetrue
andfalse
make things clearer.I don't have a problem with comments like https://github.com/Rudizind/quantum_minesweeper/blob/981d75c03476a725e759cc66876156fe88f2c478/content/solver.js#L539 but be careful to catch all these before you submit. I've marked some really embarrassing ones like this that have ended up on MMS in the past! :)
I don't love that
easySolve
callssolveBoardProbs
. This makes your code less flexible, and means that the function doesn't only do "what it says on the tin", violating the Principle of Least Surprise! How about some setup whereeasySolve
gives a return value that indicates whether it found anything, and the main program decides what to do based on that?Computing each tile's
neighbours
at the start is great, and I'm glad you've made that change. However, the code in board.js that does this could be simplified quite a bit, avoiding all those special cases. Here's some pseudo-code for the approach I'd take:I hope that's useful. I might have some other comments before we meet again, but if not, I'll see you Tuesday! We can discuss this and any other stuff then. :)
Please close this issue whenever it suits you.