4
\$\begingroup\$

I'm trying to print a diamond of asterisks of an arbitrary size. I'm new to F#, so I'm interested in virtually any feedback, but especially adherence to idioms, convention, and functional orientation.

module Diamond
open System

let rangeWithInverse (max) = 
    List.zip [ 1 .. max ] [ max .. -1 .. 1]

let listAscDesc (list) = 
    list @ (list |> List.rev |> List.skip 1)

let starsAndSpacesString (stars: int, spaces: int): string =
    String.replicate spaces " " + String.replicate ((stars * 2) - 1) "*"

let diamond =
    rangeWithInverse 18
        |> listAscDesc
        |> Seq.map starsAndSpacesString
        |> Seq.reduce (fun a b -> a + "\n" + b)

[<EntryPoint>]
let main argv = 
    diamond |> printfn "%s"
    0
\$\endgroup\$
  • 2
    \$\begingroup\$ Doesn't look half bad, but: given just the code, how do I know what it is supposed to accomplish? (An alternative approach might start n blanks, chop one blank and add two stars per line 'til "all stars" and go back.) \$\endgroup\$ – greybeard Oct 27 '16 at 5:02
  • 2
    \$\begingroup\$ Well I don't see any <- operators so it's probably pretty close to functional. \$\endgroup\$ – Der Kommissar Oct 27 '16 at 5:25
3
\$\begingroup\$

It's hard to comment on it. It seems OK to me - maybe a little to much list manipulation. I'm not sure if string concat with + is acceptable practice in F# but in general it's not. For fun I've tried with my own version with use of recursion:

let newDiamond n =
    let rec print nn f = 
        if nn <= n && nn > 0 then
            let spaces = String.replicate ((n - nn) / 2) " "
            sprintf "%s%s%s\n%s" spaces (String.replicate nn "*") spaces (print (f nn) f)
        else
            ""
    let add x = x + 2
    let subt x = x - 2
    let dn = if n % 2 = 0 then 3 else 2
    sprintf "%s%s" (print 1 add) (print (n - dn) subt)

[<EntryPoint>]
let main argv = 
    newDiamond 36 |> printfn "%s"    
    Console.ReadLine() |> ignore
    0

Version with pattern match instead of if:

let newDiamond n =
    let rec print nn f = 
        match nn with
        | x when x <= 0 || x >= n + 1 -> ""
        | _ -> let spaces = String.replicate ((n - nn) / 2) " "
               sprintf "%s%s%s\n%s" spaces (String.replicate nn "*") spaces (print (f nn) f)

    let add x = x + 2
    let subt x = x - 2
    let dn = if n % 2 = 0 then 3 else 2
    sprintf "%s%s" (print 1 add) (print (n - dn) subt)

It's possible but does it add any good?

EDIT:

A condensed solution could be:

let newDiamond n = 
    let topNs = [1..2..n]
    let diamondNs = List.append topNs (topNs |> List.rev |> List.tail)

    let createLine x = 
        sprintf "%s%s" (String.replicate ((n - x) / 2) " ") (String.replicate x "*")

    (diamondNs |> Seq.mapFold (fun strs x -> ("", sprintf "%s\n%s" strs (createLine x))) "") |> snd

However none of the solutions are very performant if diamond size is increased (> 2000) due to the concatenation of strings. In that respect a good old iterative solution is preferable:

let newDiamond n =
    let createLine = (fun x -> sprintf "%s%s" (String.replicate ((n - x) / 2) " ") (String.replicate x "*"))
    let topHalf = [ 1..2..n ]
    let indices = topHalf @ (topHalf |> List.rev |> List.tail)
    seq { for ln in indices do
            yield createLine ln } |> Seq.iter (printfn "%s")

A gentle "refactoring" of BadHeuristics code:

let rangeWithInverse max = 
    List.zip [ 1 .. max ] [ max .. -1 .. 1]

let listAscDesc list = 
    list @ (list |> List.rev |> List.tail) // tail instead of skip 1

let starsAndSpacesString (stars, spaces) =
    sprintf "%s%s" (String.replicate spaces " ") (String.replicate ((stars * 2) - 1) "*") // sprintf instead of ""+""

let diamond n =
    rangeWithInverse (n / 2)
        |> listAscDesc
        |> Seq.map starsAndSpacesString
        |> Seq.reduce (fun a b -> sprintf "%s\n%s" a b) // sprintf instead of ""+""
\$\endgroup\$
  • \$\begingroup\$ That's an interesting alt solution. Could pattern matching be used instead of if/else here? Does the preference between the two depend on the situation? RE: the string concat, it smelled a bit fishy, but using String.Format instead never occurred to me. \$\endgroup\$ – BadHeuristics Oct 27 '16 at 11:17
  • \$\begingroup\$ @BadHeuristics: about if/else and pattern matching: I think there are religions about that question, but I use if/else for two opts and match for more if it makes sense. \$\endgroup\$ – Henrik Hansen Oct 27 '16 at 11:37
  • 2
    \$\begingroup\$ not sure this is a better solution. The code is not the obvious \$\endgroup\$ – user110704 Oct 27 '16 at 12:03
  • \$\begingroup\$ I suggested this option . Although it is only comments to the code, not the algorithm. \$\endgroup\$ – user110704 Oct 27 '16 at 12:05
  • 1
    \$\begingroup\$ well, I'd use sprintf instead of String.Format \$\endgroup\$ – user110704 Oct 27 '16 at 12:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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