9.4 KiB
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 likenoneEqual :: [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 havecheckCell :: 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 (like0, 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 forMonad
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 thoughNQueens
used yourCSP
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
asSet a
, and your domain as[(a, Set b)]
(wherea
is the type of variable, andb
is the type of elements in the domain). You could probably even get away with domains that contain different types (for instance, vara
is an Int while varb
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
, notwin_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 haveai_mod
. -
There's a lot of repetition in your
PPG
data structure. Particularly aggravating is the duplication of many fields:bat1
andbat2
,bat1height
andbat2height
, 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 likesceneState 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 onbat1state
(which should also be a data type, likeBatState
), but if you turn on the standard warnings in GHCI (by pasing--ghci-options "-W"
tostack
), you'll see that this pattern is not total! It only covers the cases0
,1
, and2
, but it doesn't cover the cases of3
,4
, and so on, which are valid values ofInt
! Even though you know that the number can only be0-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 count45
lines (albeit with some white space). It's also full of harcoded constants, like185
,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''
andy'''
. They're pretty much the same function, except one computes the y-axis for thebat1
, and the other computes it forbat2
. 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 intoPlayerData
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 ofwhere
! -
Your comments are quite good, and you even used the
^--
Haddoc-style comments in various places! Nice job with that, too.