Homework-New/milestone_2_feedback.md

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.