2021-06-27 23:56:40 -07:00

## 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:

``````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 (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.

• 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.