HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Is this Bridges code F# idiomatic?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thiscodebridgesidiomatic

Problem

Introduction

I'm primarily a C# programmer, just starting out with learning F#. I found myself with a problem which felt like it was appropriate for a functional language, and now that I have the first module in place, I want to get a review before ploughing on
Problem Statement

The game Bridges works as follows:

  • The board is an NxM grid of locations



  • Each location can (but doesn't have to) contain an island which has a number written on it



  • Each island can also be connected horizontally or vertically to an adjacent island by a bridge. The adjacent island can be any distance, as long as it is directly along a compass direction.



  • There can be at most two bridges connecting a pair of islands, and bridges may not cross



  • The aim of the game is to draw bridges such that every island has a number of bridges connected to it equal to the number written on it. Additionally, every island must be reachable from every other island by the network of bridges.



                                                        

The ultimate aim of the program will be to have a Bridges solver, which can take any board and solve it by applying some algorithms and using a tree search where it can't work out a certain next move. But what I'm including for this review is just the core module, which defines the relevant types and provides the basic functions needed to interact with the game.
The Code

```
module Core

let MaxBridgesInConnection = 2

type Island = {
Position : int * int
Required : int
}

type Connection = {
Origin : Island
Destination : Island
Bridges : int
}

type Board = {
Islands : Island list
Connections : Connection list
}

let PointBetween start finish (X, Y) =
let between a b x =
(x >= a && x = b)
match start, finish with
| (0, startY), (0, finishY) -> between startY finishY Y
| (startX, 0), (finishX, 0) -> between startX finishX X
| _ -> failwith "Between must be used for a horizontal or vertical line"

l

Solution

module Core


This sounds way too general to me. Unless this code is in a namespace that you're not showing here, you should use a more descriptive name.

type Connection = {
    Origin : Island
    Destination : Island
    Bridges : int
}


I don't like that you have undirected graph, but you're calling the edges Origin and Destination, as if the edge was directed. Though I'm not sure what would be a better solution: tuples are also ordered and creating a custom "unordered pair" type just for this is an overkill.

let PointBetween start finish (X, Y) =
    let between a b x =
        (x >= a && x = b)
    match start, finish with
    | (0, startY), (0, finishY) -> between startY finishY Y
    | (startX, 0), (finishX, 0) -> between startX finishX X
    | _ -> failwith "Between must be used for a horizontal or vertical line"


I don't think this can work. Vertical line is not just a line that has both X coordinates zero, it's a line that has both X coordinates the same.

To fix the pattern, you could do something like:

match start, finish with
| (startX, startY), (finishX, finishY) when startX = finishX -> between startY finishY Y
| (startX, startY), (finishX, finishY) when startY = finishY -> between startX finishX X
| _ -> failwith "Between must be used for a horizontal or vertical line"


But that's not very nice and I think it would be better using if-elif-else:

let (startX, startY) = start
let (finishX, finishY) = finish

if startX = finishX then between startY finishY Y
elif startY = finishY then between startX finishX X
else failwith "Between must be used for a horizontal or vertical line"


match (a.Origin.Position, a.Destination.Position, b.Origin.Position, b.Destination.Position) with 
| (aOrig, aDest, bOrig, bDest) ->


I don't think you should be using pattern matching like this. If you want to assign to multiple variables at the same time, you can use let:

let (aOrig, aDest, bOrig, bDest) =
    (a.Origin.Position, a.Destination.Position, b.Origin.Position, b.Destination.Position)


PointBetween aOrig aDest bOrig
&& PointBetween aOrig aDest bDest
&& PointBetween bOrig bDest aOrig
&& PointBetween bOrig bDest aDest


You could get rid of some of the duplication here by writing a list of tuples of the parameter combinations and then calling List.forall PointBetween:

[aOrig, aDest, bOrig; aOrig, aDest, bDest;
 bOrig, bDest, aOrig; bOrig, bDest, aDest] |>
    List.forall PointBetween


Though this requires that you declare PointBetween in tupled form:

let PointBetween(start, finish, (X, Y)) =


I have also considered generating the sequence of points, instead of hard-coding it, but the best I could figure out is:

[ for (con1, con2) in [a, b; b, a] do
  for v in [con2.Origin; con2.Destination] do
  yield (con1.Origin.Position, con1.Destination.Position, v.Position) ]


I think think makes the structure more obvious, but it's also more complicated, so it's probably the worse option.

IslandConnections board island
|> Seq.sumBy (fun connection -> connection.Bridges)
|> (>) island.Required


That |> (>) part looks confusing to me. I would probably write this as:

let existingConnections = 
    IslandConnections board island
    |> Seq.sumBy (fun connection -> connection.Bridges)
existingConnections > island.Required

Code Snippets

module Core
type Connection = {
    Origin : Island
    Destination : Island
    Bridges : int
}
let PointBetween start finish (X, Y) =
    let between a b x =
        (x >= a && x <= b) || (x <= a && x >= b)
    match start, finish with
    | (0, startY), (0, finishY) -> between startY finishY Y
    | (startX, 0), (finishX, 0) -> between startX finishX X
    | _ -> failwith "Between must be used for a horizontal or vertical line"
match start, finish with
| (startX, startY), (finishX, finishY) when startX = finishX -> between startY finishY Y
| (startX, startY), (finishX, finishY) when startY = finishY -> between startX finishX X
| _ -> failwith "Between must be used for a horizontal or vertical line"
let (startX, startY) = start
let (finishX, finishY) = finish

if startX = finishX then between startY finishY Y
elif startY = finishY then between startX finishX X
else failwith "Between must be used for a horizontal or vertical line"

Context

StackExchange Code Review Q#68011, answer score: 7

Revisions (0)

No revisions yet.