196 lines
9.4 KiB
Markdown
196 lines
9.4 KiB
Markdown
## Orange Sudoku
|
|
* Could you perhaps avoid the (unsafe) list indexing using (!!) in Sudoku? Although it's difficult
|
|
to represent the finite-length list of elements in Haskell, you may be able to tweak your representation
|
|
of the sudoku puzzle to avoid having to use (!!). For instance, what if you had:
|
|
|
|
```Haskell
|
|
type Array2d a = [[a]]
|
|
type Cell = Array2d Int
|
|
type Sudoku = Array2d Cell
|
|
```
|
|
|
|
__Disclaimer: all type signatures and functions below are written without a Haskell
|
|
interpreter at hand. There are probably errors - I can't always write Haskell without
|
|
feedback.__.
|
|
|
|
That is, your top-level data structure would be a 3x3 grid of 3x3 "cells". You could then extract
|
|
your `mapAllPairsM_ ...` function into something like `noneEqual :: [a] -> CSP ()` (the exact
|
|
signature is not correct since I didn't have enough time to study the types of functions in the CSP
|
|
library. Then, you can have
|
|
|
|
```
|
|
checkCell :: Cell -> CSP ()
|
|
checkCell = noneEqual . concat
|
|
|
|
checkCells :: Sudoku -> CSP ()
|
|
checkCells = mapM_ (mapM_ checkCell)
|
|
```
|
|
|
|
Of course, you still need to extract rows. You can do it with something like the following:
|
|
|
|
```
|
|
mergeRow :: [Cell] -> [[Int]]
|
|
mergeRow = foldr (zipWith (++)) []
|
|
|
|
allRows :: Sudoku -> [[Int]]
|
|
allRows = concatMap mergeRow
|
|
```
|
|
|
|
And then, your two remaining constraints can be solved as:
|
|
|
|
```
|
|
checkRows :: Sudoku -> CSP ()
|
|
checkRows = mapM_ noneEqual . allRows
|
|
|
|
checkColumns :: Sudoku -> CSP ()
|
|
checkColumns = mapM_ noneEqual . transpose . allRows
|
|
```
|
|
|
|
And finally, your entire Sudoku checker constraint would be:
|
|
|
|
```
|
|
check :: Sudoku -> CSP ()
|
|
check s = checkCells s >> checkRows s >> checkColumns s
|
|
```
|
|
|
|
Look ma, no mention of size whatsoever! In fact, there's no mention of numbers at all in this code.
|
|
There is, of course, the assumption in all the above code that your cells are always NxN.
|
|
* Your `cells` function seems to be unused. I actually prefer this function to the rest of your code,
|
|
because it doesn't have as many hardcoded numbers (like `0, 3, 6,`). On the other hand, your actual
|
|
solver hardcodes a _lot_ of numbers, which means that your code is not generalizeable to higher
|
|
dimensions, even though there's nothing inherently difficult about generalizing the sudoku solving problem.
|
|
* I really like your definition of `mapAllPairsM_`! This seems like the perfect level of abstraction for me,
|
|
and the typeclass constraint for `Monad` makes it more general. Nice!
|
|
* It looks like you actually implemented your own constraint solver in `CSP.hs`. Why didn't you use this for
|
|
your Sudoku itself? It seems as though `NQueens` used your `CSP` module, and it seems like Sudoku _should_
|
|
work with binary constraint systems (each two variable has to take on assignments (1, 2), (2, 1), (1, 3), ...).
|
|
* In general, I don't know if I'm a fan of using integers and Haskell's range syntax for assigning variables.
|
|
It just seems to.... hardcoded? Maybe abstraction has fried my brain, and I'm incapable of perceiving any
|
|
type that is not polymorphic, but I _do_ think it should be possible, to, say, use string / character / byte
|
|
variables. You could then represent `vars` as `Set a`, and your domain as `[(a, Set b)]` (where `a` is the
|
|
type of variable, and `b` is the type of elements in the domain). You could probably even get away with
|
|
domains that contain different types (for instance, var `a` is an Int while var `b` is a String) if you
|
|
used [existential types](https://wiki.haskell.org/Existential_type) (did we learn these in class? I saw
|
|
other groups using them, but I don't remember hearing Eric talk about them...).
|
|
* Hmm, your `load` function is undefined. You probably want to implement that in time for the final submission.
|
|
* You clearly did great work with the constraint solver! Your `NQueens` solution is very short in Haskell.
|
|
I also really enjoy watching the results print out on the screen. It is quite slow though; is that inherent
|
|
to CSP, or do you think your implementation could use some work? You should check out how to do _profiling_
|
|
in Haskell - it's one of the most important skills industry Haskell jobs seem to look for.
|
|
|
|
Nice work, and have a great day!
|
|
|
|
## Ping Pong
|
|
* I wasn't able to run your code, becasue I am on Linux and your instructions did not include
|
|
information about how to set up on a Linux machine. It's no problem, though - I know what
|
|
a pain it is to distribute graphics libraries etc. to users.
|
|
* As soon as I open `Data.hs`, my Haskell linter complains: you're not using camel case!
|
|
The proper varible naming convention is, for example, `winScore`, not `win_score`. It's _very_
|
|
uncommon to use anything else, and when other things _are_ used, it's usually for good reason!
|
|
|
|
In fact, it seems like your code does not itself follow a consistent format. You have `sceneState`,
|
|
but then you have `ai_mod`.
|
|
* There's a lot of repetition in your `PPG` data structure. Particularly aggravating is the
|
|
duplication of many fields: `bat1` and `bat2`, `bat1height` and `bat2height`, and so on.
|
|
Could you, perhaps, define a second data structure that contains all the common information?
|
|
|
|
```
|
|
data PlayerData = PlayerData
|
|
{ bat :: Float
|
|
, batState :: Int
|
|
, batHeight :: Float
|
|
, score :: Int
|
|
}
|
|
```
|
|
|
|
And then, you'd have:
|
|
|
|
```
|
|
data PPG = Game
|
|
{ ballPos :: (Float, Float) -- ^ Pong ball (x, y) Position.
|
|
, ballVel :: (Float, Float) -- ^ Pong ball (x, y) Velocity.
|
|
, sceneState :: Int -- 0: Instruction, 1: Play, 2: End
|
|
, ballspeed :: Float
|
|
, ai_mod :: Int
|
|
, player1 :: PlayerData
|
|
, player2 :: PlayerData
|
|
} deriving Show
|
|
```
|
|
* It seems like you're using integers to represent states! I see the comment:
|
|
|
|
```
|
|
-- 0: Instruction, 1: Play, 2: End
|
|
```
|
|
|
|
This is not at all idiomatic in Haskell! It is very easy to define data types in Haskell,
|
|
and that's precisely what you should do:
|
|
|
|
```
|
|
data SceneState = Instruction | Play | End deriving (Eq, Show)
|
|
|
|
-- ...
|
|
, sceneState :: SceneState
|
|
-- ---
|
|
```
|
|
|
|
Instead of using `sceneState game == 0`, you'd then use something like
|
|
`sceneState game == Instruction`, or better yet, you'd use pattern matching! Pattern matching
|
|
really _is_ the bread and butter of Haskell programming. I see you do use pattern matching
|
|
on `bat1state` (which should _also_ be a data type, like `BatState`), but if you turn
|
|
on the standard warnings in GHCI (by pasing `--ghci-options "-W"` to `stack`), you'll
|
|
see that this pattern __is not total__! It only covers the cases `0`, `1`, and `2`,
|
|
but it doesn't cover the cases of `3`, `4`, and so on, which are valid values of `Int`!
|
|
Even though you _know_ that the number can only be `0-2`, it's much better practice
|
|
(and far more idiomatic) to move these kind of invariants into our type system, so
|
|
that it's _impossible_ to write incorrect code. I think the general name for this approach
|
|
is [make illegal states unrepresentable](https://fsharpforfunandprofit.com/posts/designing-with-types-making-illegal-states-unrepresentable/).
|
|
* Your `render` function is very long! I count `45` lines (albeit with some white space). It's
|
|
also full of harcoded constants, like `185`, `110`, and so on.
|
|
This is the [magic number](https://en.wikipedia.org/wiki/Magic_number_(programming%29) antipattern!
|
|
You can try extracting them into some constants, or better yet, positioning them relatively (using
|
|
information such as text height, screen height, and some basic typography-type math) so that
|
|
it fits many screen sizes and configurations!
|
|
* You may be usign too many parentheses; here's a screenshot of my editor viewing one of your source code files!
|
|
As you can see, there's quite a lot of yellow, mostly from unnecessary uses of `(` and `)`.
|
|
* Not your code duplicaton in `y''` and `y'''`. They're pretty much the same function, except one
|
|
computes the y-axis for the `bat1`, and the other computes it for `bat2`. This amount of code
|
|
duplication is a smell - you would be able to reduce this duplication to a single function if
|
|
you were to extract your bat data into `PlayerData` records as I mentioned in my earlier comment.
|
|
* This may be controversial, but instead of using `if then/else if` chains as you do, you can try
|
|
pattern matching on the boolan values (maybe something like this):
|
|
|
|
```
|
|
case (leftout (ballPos game), rightout (ballPos game)) of
|
|
(True, _) -> game {p1score = (p1score game) + 1, ballPos = (0, 0), ballVel = (-30, -40) }
|
|
(_, True) -> game {p2score = (p2score game) + 1, ballPos = (0, 0), ballVel = (-30, -40) }
|
|
_ -> game
|
|
```
|
|
|
|
I think this is easier to follow than variously indented `if`/`else` chains.
|
|
|
|
* In the segment of code above, you are also repeating your code for resetting the ball position
|
|
and velocity. What about a function:
|
|
|
|
```
|
|
resetBall :: PPG -> PPG
|
|
resetBall game = game { ballPos = (0, 0), ballVel = (-30, -40) }
|
|
```
|
|
|
|
Then, the above code becomes:
|
|
|
|
```
|
|
case (leftout (ballPos game), rightout (ballPos game)) of
|
|
(True, _) -> resetBall $ game {p1score = (p1score game) + 1 }
|
|
(_, True) -> resetBall $ game {p2score = (p2score game) + 1 }
|
|
_ -> game
|
|
```
|
|
|
|
And now, it's much clearer what each case does! If the ball is out
|
|
on either side, you reset its position, and add points to the
|
|
other player!
|
|
|
|
* In `outofBound` and elsewhere, nice use of `where`!
|
|
|
|
* Your comments are quite good, and you even used the `^--` Haddoc-style
|
|
comments in various places! Nice job with that, too.
|