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:

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