2
\$\begingroup\$

Recently I started to learn Haskell. I did this by making exercises on the Internet. The problem of making these exercises is that I never know if I solved it the correct way. I currently wrote a function which I think looks really ugly, but I don't know how to improve it. I hope that someone could give me some tips to improve it.

In this exercise I need to make a Tic-Tac-Toe game.

The question of the function I wrote is as follows:

Exercise 8. Write a function moves :: Player -> Board -> [Board] that, given the current player and the current state of the board, returns all possible moves that player can make expressed as a list of resulting boards. (For now, you should continue making moves, even if one of the players has already won.)

I wrote the following code:

moves :: Player -> Board -> [Board]
moves p ((a,b,c),(d,e,f), (g,h,i)) = (moves' a (((symbol p),b,c), (d,e,f), (g,h,i))) ++
                                     (moves' b ((a,(symbol p),c), (d,e,f), (g,h,i))) ++
                                     (moves' c ((a,b,(symbol p)), (d,e,f), (g,h,i))) ++
                                     (moves' d ((a,b,c), ((symbol p),e,f), (g,h,i))) ++
                                     (moves' e ((a,b,c), (d,(symbol p),f), (g,h,i))) ++
                                     (moves' f ((a,b,c), (d,e,(symbol p)), (g,h,i))) ++
                                     (moves' g ((a,b,c), (d,e,f), ((symbol p),h,i))) ++
                                     (moves' h ((a,b,c), (d,e,f), (g,(symbol p),i))) ++
                                     (moves' i ((a,b,c), (d,e,f), (g,h,(symbol p))))

moves' :: Field -> Board -> [Board]
moves' c m = if c == B then [m] else []

Is there a way to improve my code?

\$\endgroup\$
2
  • \$\begingroup\$ I did not know it exist. I will ask it there. Thanks. \$\endgroup\$
    – maffh
    Commented Jul 31, 2015 at 14:15
  • \$\begingroup\$ Sorry, I will do that next time. \$\endgroup\$
    – maffh
    Commented Jul 31, 2015 at 19:39

1 Answer 1

6
\$\begingroup\$

First of all, you can use a where binding to clear this up:

moves :: Player -> Board -> [Board]
moves p ((a,b,c),(d,e,f), (g,h,i)) = (moves' a ((s,b,c), (d,e,f), (g,h,i))) ++
                                     (moves' b ((a,s,c), (d,e,f), (g,h,i))) ++
                                     (moves' c ((a,b,s), (d,e,f), (g,h,i))) ++
                                     (moves' d ((a,b,c), (s,e,f), (g,h,i))) ++
                                     (moves' e ((a,b,c), (d,s,f), (g,h,i))) ++
                                     (moves' f ((a,b,c), (d,e,s), (g,h,i))) ++
                                     (moves' g ((a,b,c), (d,e,f), (s,h,i))) ++
                                     (moves' h ((a,b,c), (d,e,f), (g,s,i))) ++
                                     (moves' i ((a,b,c), (d,e,f), (g,h,s)))
    where
        s = symbol p

Next, I would recommend changing your Board type to use a flat list instead of nested tuples, it's a lot easier to work with:

type Board = [Field]

For whatever your Field type is (for future reference please include all relevant definitions in your questions on SO, this goes for any language). Now you can write this a bit differently:

moves :: Player -> Board -> [Board]
moves p [a, b, c, d, e, f, g, h, i] = moves' a [s, b, c, d, e, f, g, h, i] ++
                                      moves' b [a, s, c, d, e, f, g, h, i] ++
                                      moves' c [a, b, s, d, e, f, g, h, i] ++
                                      moves' d [a, b, c, s, e, f, g, h, i] ++
                                      moves' e [a, b, c, d, s, f, g, h, i] ++
                                      moves' f [a, b, c, d, e, s, g, h, i] ++
                                      moves' g [a, b, c, d, e, f, s, h, i] ++
                                      moves' h [a, b, c, d, e, f, g, s, i] ++
                                      moves' i [a, b, c, d, e, f, g, h, s]
    where
        s = symbol p

Now I recognize a zipWith here between the first and second arguments to moves', although we'll need a concat after the zipWith is done:

moves :: Player -> Board -> [Board]
moves p board@[a, b, c, d, e, f, g, h, i]
    = concat
    $ zipWith moves'
        board
        [[s, b, c, d, e, f, g, h, i],
         [a, s, c, d, e, f, g, h, i],
         [a, b, s, d, e, f, g, h, i],
         [a, b, c, s, e, f, g, h, i],
         [a, b, c, d, s, f, g, h, i],
         [a, b, c, d, e, s, g, h, i],
         [a, b, c, d, e, f, s, h, i],
         [a, b, c, d, e, f, g, s, i],
         [a, b, c, d, e, f, g, h, s]]
    where
        s = symbol p

Now, this still leaves the big block of different boards. There is definitely a pattern here. If you have a replace function with the type replace :: Int -> a -> [a] -> [a] (I'll leave this as an exercise to the reader) then you could write this as

[replace 0 s board,
 replace 1 s board,
 replace 2 s board,
 replace 3 s board,
 replace 4 s board,
 replace 5 s board,
 replace 6 s board,
 replace 7 s board,
 replace 8 s board]

Or more simply

map (\i -> replace i s board) [0..8]

So your entire function becomes

moves p board
    = concat $ zipWith moves' board
    $ map (\i -> replace i s board) [0..8]
    where
        s = symbol p
        replace i a xs = undefined
        moves' B m = [m]
        moves' _ _ = []
\$\endgroup\$
4
  • 1
    \$\begingroup\$ If you make i the last argument to replace, then you can partially apply it instead of using a lambda (note that currently, the order in the application does not match that implied by the un-definition). \$\endgroup\$
    – pat
    Commented Jul 31, 2015 at 15:23
  • \$\begingroup\$ @pat I thought about it, but replace is a pretty general use case function and then it would just look weird to me with the index as the last argument. I don't think it's a terrible thing to have to use a lambda now and again. Thanks for spotting that typo, I've got that fixed now. \$\endgroup\$
    – bheklilr
    Commented Jul 31, 2015 at 15:31
  • \$\begingroup\$ Thank you for the clear explaination! I think your way is much better than mine! There is only one problem for me. There is huge part of code already written for me, so if I change the type I need to change all the code. My purpose was only to improve my ownfunction. Sorry for not telling! When I am done with the whole exercise, I am going to improve it with your solution. \$\endgroup\$
    – maffh
    Commented Jul 31, 2015 at 19:39
  • \$\begingroup\$ @maffh Yeah, the real key here is switching from nested tuples to a list for the board type, it's just a lot easier to work with with lots of built-in functions. In some ways it's more harmful since you can create boards with the wrong number of elements, but this can be alleviated by using a newtype wrapper along with a mkBoard :: [Field] -> Board function which ensures that you have only 9 Fields. However for an exercise like this one there isn't much of an issue. \$\endgroup\$
    – bheklilr
    Commented Jul 31, 2015 at 19:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.