Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've written a simple program to test out the capabilities of netwire (or rather, how to write programs using netwire). The goal is to have an application that shows a window, starting out as completely filled with red. Pressing space should switch from red to blue and vice versa. Pressing escape or closing the window should terminate the program. I've been able to write something that works (using Allegro, but that doesn't matter too much - the Allegro part can't be simplified, at least not by much), but it's gotten a bit lengthy for something that sounds so simple.

Here's the code (if there's anything particularly unidiomatic, I'd be glad if someone could tell me that too):

{-# LANGUAGE OverloadedStrings #-}

module Main(main) where

import Control.Wire hiding (when)
import Prelude hiding ((.),id)
import qualified Graphics.UI.Allegro.Raw as Al
import Data.Monoid
import Control.Monad.Reader
import qualified Data.Set as S
import Data.Set (Set)

iff True  a _  = a
iff False _ b  = b

data Game = Game {
    keyState :: Set Al.Key,
    isClosed :: Bool
}

data Result = GameColor Al.Color | Quit

colRed = Al.Color {Al.colorR = 1.0, Al.colorG = 0.0, Al.colorB = 0.0, Al.colorA = 1.0}
colBlue = Al.Color {Al.colorR = 0.0, Al.colorG = 0.0, Al.colorB = 1.0, Al.colorA = 1.0}

initG = Game { keyState = S.empty, isClosed = False }
addKey (g@ Game {keyState = s}) k = g {keyState = S.insert k s}

-- Produce when event occurs, inhibit otherwise
gameEvent :: Monoid e => (Game -> Bool) -> Event e (ReaderT Game IO) a
gameEvent f = mkFixM $ \_ a ->
  iff <$> asks f <*> (return $ Right a) <*> (return $ Left mempty)

windowClosed = gameEvent isClosed

keyPressed k = gameEvent (S.member k . keyState)

doQuit = pure Quit . (keyPressed Al.KeyEscape <|> windowClosed)
color = doColorRed


-- The once prevents infinite recursion in case space was pressed
doColorRed = pure (GameColor colRed) . (once <|> notE (keyPressed Al.KeySpace)) -->
              doColorBlue
doColorBlue = pure (GameColor colBlue) . (once <|> notE (keyPressed Al.KeySpace)) -->
                doColorRed

colorApp :: Wire () (ReaderT Game IO) a Result
colorApp = doQuit <|>
            color

main = do
  Al.initialize
  Al.installKeyboard
  Just window <- Al.createDisplay 800 600
  Al.setTargetBackbuffer window
  Just q <- Al.createEventQueue
  t <- Al.createTimer $ 1/60
  Al.registerEventSource q =<< Al.getKeyboardEventSource
  Al.registerEventSource q =<< Al.getDisplayEventSource window
  Al.registerEventSource q =<< Al.getTimerEventSource t
  Al.startTimer t
  loop colorApp clockSession q
  Al.destroyEventQueue q
  Al.destroyDisplay window where
    loop w session q = do
      game <- handleEvents initG q
      (mx, w', session') <- runReaderT (stepSession w session ()) game
      case mx of
         Left ex -> putStrLn $ "Inhibited: " ++ show ex -- this should never happen
         Right (GameColor c) -> do
           Al.clearToColor c
           Al.flipDisplay
           loop w' session' q
         Right Quit -> return ()
    handleEvents g q = do 
      ev <- Al.waitForEvent q
      case ev of
        Al.KeyDownEvent{Al.eventKeycode = k} -> handleEvents (addKey g k) q
        Al.Timer{}                           -> return g
        Al.DisplayClose{}                    -> return g{isClosed = True}
        _                                    -> handleEvents g q
share|improve this question
    
Wow, that does really look like a heck of a lot of haskell for what it does, and it's also kind of hard to penetrate... Normally I find haskell a bit more readable, I wonder if this Allegro API might not be the best quality if it causes consumer code to do this much... Alternatively it may be that I'm unfamiliar with Allegro's API that makes this a bit tricky appearing to me –  Jimmy Hoffa Jun 25 '13 at 4:36
    
I think filling out some type signatures on what you have and designing some for what you want, then filling in the implementation based on the signatures you want to work with as a consumer will benefit you a lot and find you breaking these up and modeling this stuff a little more concisely –  Jimmy Hoffa Jun 25 '13 at 5:16
    
@JimmyHoffa It's a raw binding to a C API - there's a bit of setup code involved, but after that it's pretty straight forward. –  Cubic Jun 25 '13 at 9:25
    
I understand that, I'm just not altogether clear that you're getting a nice clean API from it in that to consume it each step you perform requires so much code; though I've not worked with any library's like it so perhaps this is par for the course on API's to accomplish such functionality. Either way try some of the clean up I talked about data modeling your system, and then I'm curious what it looks like; I don't think I can mentally penetrate any further until it's had some of that clean up. –  Jimmy Hoffa Jun 25 '13 at 17:00

1 Answer 1

Your iff function would be better expressed with <|> or msum. They're for making a choice, so one of them ought be used in place of iff, I'm not sure where the True/False values are feeding out into iff, but I would maybe change them to feed out something that's a Monoid type or MonadPlus instead of Bool then use the appropriate choice technique.

You named a data type which appears to be the purpose of being a state Result, was this supposed to be a Command, Instruction, NextState, or Update instead perhaps? Result seems like a confusing name to me for what I (think?) that is.

data Update = Color (Float, Float, Float) | Quit | Reset | etc | etc | whatever
type Command = (Game -> Update -> IO Game) -- Presuming those functions which execute changes like doColorRed return an IO monad

I think you're missing a cohesive set of data models to present your system and it's possible states for which you could write functions to alter... When working with any UI presentation, it's usually best to create a model of your system and a separate view which responds to your model.

I would change:

  case mx of
     Left ex -> putStrLn $ "Inhibited: " ++ show ex -- this should never happen
     Right (GameColor c) -> do
       Al.clearToColor c
       Al.flipDisplay
       loop w' session' q
     Right Quit -> return ()

To:

toColor (r,g,b) = Al.Color {Al.colorR = r, Al.colorG = g, Al.colorB = b, Al.colorA = 1.0}
ProcessCommand g (Color c) = Al.clearToColor (toColor c) >> Al.flipDisplay >> (gameLoop g)
ProcessCommand g Quit = return ()

then where you do your case, make sure your Game data type (represented as g above) has those w', session', and q variables as a part of your Game data type so the gameLoop function can execute that loop given any Game

and your case can take advantage of the exception monad instead of doing a case...

fmap (processCommand g) (mx <|> Right $ someFunctionThatTakesMxAndCreatesACommandToPrintTheError mx)

Give some of these techniques a try and see how it cleans up for ya. After that perhaps you'll see a higher level design appear that will make the domain a little easier to model and approach.

share|improve this answer
    
Could you expand your idea of iff expressed as <|> for (Bool,a)? –  Petr Pudlák Jun 25 '13 at 7:26
    
@PetrPudlák perhaps that's a dumb idea, I'm just thinking either <|> or msum are for making a choice, so one of them ought be used in place of iff, I'm not sure where the True/False values are feeding out into iff, but I would maybe change them to feed out something that's a Monoid type or MonadPlus instead of Bool then use the appropriate choice technique –  Jimmy Hoffa Jun 25 '13 at 16:41

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.