Homework-New/milestone_1_feedback.md

# Jack Attack

* Your comments are very thorough! Have you, however, consider the special
"haddock" syntax for your documentation? For instance, in your `Object` record,
you clearly describe what each field does. If you used the "haddock" syntax
(maybe something like `--^ comment goes here`), you'd be able to generate
static documentation pages much like those you see on hackage or stackage.
Those are useful because they allow the user to browse the documentation
for your package from the web before deciding to install and use it. And,
of course, I think it's nicer to look at a web page than comments!

* There's a lot of duplicate functionality in your `doTimedTransform`
  function compared to your `doTransform` function. It seems almost
  as though there should be _one_ function that takes care of both
  the interpolation and the transformation; perhaps `doTransform`
  can take an argument `0.0..1.0` of "progress", and go from there.
  This will save you the trouble of unpacking and repacking your
  `Transformation` objects. Even if you don't go for that approach,
  however, there's still an improvement you can make. Rather than making
  your `doTimedTransform` function do _everything_, you may want to define
  a function
  
  ```Haskell
  tween :: Object -> Float -> Transformation -> Transformation
  ```

  This function would do _kind of_ like what your `doTimeTransform` does:
  it would interpolate the transformation given the current object and
  the "progress" within the transformation. The difference, though,
  is that it will not actually _apply_ the transformation, but create
  a new one (much like you're already doing in `doTimedTransform` anyway).
  You would then be able to feed this transformation into `doTransform`,
  and it would work as expected. The advantage here is that you won't have
  to be writing duplicate code for stuff like:

  ```
  doTransform (Combine xs) $ doTransform x obj
  doTimedTransform (seconds, x) (doTimedTransform (seconds, Combine xs) obj elapsed) elapsed
  ```
  
  There's a fairly clear similarity (and consequently duplication) here, and
  that's what I think my approach would suggest.

  And there's one more thing: does `tween` _really_ need access to the object
  being animated? It sure seems to if your transformations are "absolute",
  like "move to (1,2)". This "absoluteness" of transformations forces you to
  use `obj` in `doTimedTransform`, and would also force you to use it in
  `tween`. But if your transformations were _delta based_ ("move left 1, up t2"), your `tween` could be only a function of the current "progress" `Float`:

  ```
  tween :: Float -> Transformation -> Transformation
  ```

  This may help clean up some of the code, and it also allows for an
  interesting experiment: what if you compose `tween` with various
  functions `Float -> Float`? For instance, you could compose it with
  `\x -> x * x`, which I think would make your animation start out slow
  and speed up at the end. You may be able to use this to play around
  with various ways of animating your objects that are used in "actual"
  animation: from what I know, linear transformations are not as common
  as, say, sigmoid ones (animation starts slow, gets fast in the middle,
  and slows down again at the end).

* I would try to factor out the "seconds" part of your calculations as soon
as possible. In my opinion, it would be clearer to pass around a "percentage
completed" float, especially if you don't have any other dependence on
the current time.

* Cody is right, you do have a couple of places that can be used by folds
  or other higher order functions. For instance, `getAniLength` is just
  `sum . map fst`, and the `Combine` case of `doTransform` is something
  like `foldr (flip doTransform) obj (x:xs)`.

* Are transformations a monoid? I suspect that they are, with `mempty = Wait`,
`mappend x y = Combine [x,y]`, and `mconcat = Combine`. I don't know
if this will be useful, but it's always nice to have access to more
standard library functions!

* I'm not sure I follow the `AST` vs `Lib` modules. They both seem
to have _something_ in mind, but I can't quite figure out how they work
together. Does the `AST` eventually produce a `Transformation`? I just
don't really know what to make of it!

About your design questions:

* Q1: It would be cool if you had a slider that lets you play
 the animation back and forward (or at an arbitrary point in time).
 I think it may help in debugging:

 > Why did my object just make a 360 and walk away?

 It would be even better if you could label your transformations
 with the function / piece of code that induced them, so that
 at any point, you can kind of see what part of your code is
 causing the current transformation.

* Q2: At the implementation level, you may need to replace the
  `Float` argument of `doTransform` with an `Env` argument,
  which may be a record-like thing that contains the current
  mouse position, the current time, and so on. You'd then
  thread it as before.

  At a language level, things are more interesting. Have you
  looked into [functional reactive programming](https://en.wikipedia.org/wiki/Functional_reactive_programming)?
  You may be interested in encoding transformations declaratively, given, say
  the current mouse position, time, etc. as input. Your programs would then
  be descriptions of how to convert these inputs into figures on the screen.

  [Elm](elm-lang.org) is a language that implements quasi-FRP. It doesn't deal with
  continuous inputs like you would be, but it encodes programs as transition
  functions (`Event -> State -> State`). You may be interested in adapting
  a similar approach - it's a nice way to deal with "imperative" environments
  from a functional language.

* Q3: This is a tough one! I think you can come up with a textual functional
language that can be used for your domain, but I'd be thrilled to see how
you may incorporate visual (scratch-like) elements into your project. At this
point, though, I can't help but wonder: would you have time to implement something
like Scratch-like blocks? We're in week 8, after all. That would be a very graphical
type of user interface, and I can't personally imagine defining a UI from scratch,
in Haskell, using only graphics library primitives. Of course, you're probably
more versed than I am in Haskell's graphics tooling, but it seems to me like
a fairly daunting task.

# Escape

* I played with your game for a bit, pretty cool! I think you did a nice job with the textual
  users interface; it feels pretty much like I'd expect a command line interface to work.
  I did run into an error on my first playthrough, and decided not to try again: `Prelude.head: empty list`.
  This happened because I typed `search` without any arguments. Now, I understand that
  what I did was an incorrect command, but crashing the game doesn't seem like the right thing to do!
  `head` is a dangerous function; I think that if Haskell were redesigned today, from the ground
  up, this function would no longer have type `[a] -> a`. In general, exceptions in Haskell are
  a bit of an antipattern, because they're not indicated by the types (how could you know `head` throws
  an exception), but also because they're cumbersome to work with!

  Languages defined after Haskell actually define `head` with a `Maybe`. Here's the [Idris version](https://github.com/idris-lang/Idris-dev/blob/a13caeb4e50d0c096d34506f2ebf6b9d140a07aa/libs/prelude/Prelude/List.idr#L128-L130),
  and here's the [Elm version](https://package.elm-lang.org/packages/elm/core/latest/List#head). You
  may want to define your own head function, `safeHead :: [a] -> Maybe a`! You can then use pattern
  matching or the `maybe` function to properly handle the error that occurs when the user doesn't give
  the command enough arguments. You could, for instance, print "Search what?" when a user types in "search".

* This is a minor nitpick, but I'm pretty sure `snake_case` is not the preferred standard
  for Haskell variable names. Your records (and module names!) should be in `camelCase`.
  In fact, my linter plugin _covers_ the screen with warnings about style when it sees your code!
* `Funcfunctor` is a funny name for a typeclass. It seems to imply that `t` is a functor; however,
  `Functor` is a type constructor class, whereas your code treats `t` as just a type, not a type
  constructor! In fact, I'm really not sure what to make of the word `Funcfunctor`. What does
  it mean?
* On the other hand, I think that this is a good application of type classes. If you want to
  print a variety of different objects on the screen in your command line game, defining a common
  type class with the various properties of these objects is a great idea. You can certainly
  get a very consistent interface this way!
* In `Escape_funs`, a lot of your pattern matching code can be significantly simplified! You always write
  out stuff like `Object {object_id=id, object_name=...}`, but use only one of these variables (for instance, `id`).
  Instead, you can _only_ write `Object {object_id=id}` -- that's it! You don't need to manually enumerate every single
  key in your records to pattern match on them.

  A cute fun fact of this notation: it's "preferred style" according to Haskell's linter to rewrite code like `(Constructor _ _ _ _)`
  (pattern matching on a data type and ignoring all its fields) as `Constructor{}`.
* `foldr (||) False (map (==(get_key obj)) list)` is good use of a fold a map, but it also
  corresponds to the Haskell function [`any`](http://zvon.org/other/haskell/Outputprelude/any_f.html): 
  `any (==(get_key obj)) list`.
* Notice how your `run_code` is a bit fragile: your `help` function prints out all the commands available
  to the user, while `run_code` actually implements these commands. It must be very easy to
  edit `run_code` to add a new command, while forgetting to update `help`. You may be able
  to work around this by defining a `Command` data structure: maybe it'll have a `description`
  field that you print to the user, and another `action` filed that's maybe some monad like `IO ()`
  that actually performs the actions that your command does. Then, you can have a global
  list of commands `commands :: Map String Command`, which contains all the allowed commands
  in your game. Your `help` function would go through this `commands` variable and print
  out the `description` field of every available command; on the other hand, the `run_code`
  function would read a command from the user, look it up in `commands`, and try to execute
  the `action` field.
* Notice that you're explicitly handling your application's state: you will need to pass
  the correct house to `run_code` every time, which means that for each command, you need
  a separate call to `run_code` with whatever house that command puts your player in.

  An alternative approach is to handle state _implicitly_. You won't need to take a `house`
  argument explicitly, and you therefore won't have to explicitly pass it to your recursive
  calls to `run_code`. In your case, this would be best done with a `State` monad, since
  the current location of the player is, well, a piece of state.

  The question, though, becomes: how the heck do you incorporate a state monad into your
  `IO`-based code? The answer to that is the `StateT` _monad transformer_. We haven't learned
  about these in class just yet, but we will soon, and I think it would serve you well! Instead
  of making your code of type `IO ()` (what it currently is), you'd use something like
  `StateT [Room] IO ()`. Your calls to `putStrLn` would have to go through a `liftIO`,
  but you'd also gain access to functions `get` and `put`, which are the functions of the
  State monad that we have covered in class. Then, whenever your user goes to a different
  house, you'd run `put new_house`, and whenever you need to check the current location
  of the player, you'd run `get`. 

  You may then incorporate other monad transformers into this implementation; I think
  of interest to you would be `ExceptT` (which is __not the same as Haskell's exceptions
  that I mentioned in my first bullet point!__), which might help you exit out of your
  code when your player types "exit".
* By the way, you should probably explicitly type out the signatures of your function;
  I think that's considered good style. For instance, for `run_code`, you'd write:
  
  ```
  run_code :: Player -> [Room] -> IO ()
  run_code plauer house = ...
  ```

Your questions:

* Q1: Hey, I brought up `StateT` and `ExceptT` and monad transformers in general earlier!
  Check them out. I think you may find them useful for what you're doing. The only difficulty
  is that you really need to see a few examples of code before-and-after the use of State
  and Exception monads to be comfortable in applying them (at least for me! It took me a while
  to properly understand monads, and more time to understand monad transformers).

* Q2: Haskell's `IO` monad has `readFile` and `writeFile` methods, but these may be a bit
  basic for your taste. You may be interested in various formats like [INI](https://hackage.haskell.org/package/ini-0.4.1/docs/Data-Ini.html)
  or even JSON (for which you can use something like [Aeson](https://hackage.haskell.org/package/aeson-1.5.6.0/docs/Data-Aeson.html),
  especially its `encodeFile` and `decodeFile` methods).


I'm running out of steam writing this comment, but I may post a follow up if I think of anything else!

__Eric, if you're reading this:__ have I been evangelizing monad transformers too much? I feel like
I have been. It would be good to know if I'm being overzealous; everything starts to look like a nail
when all you have is a hammer!