21
Overall I think it's fine.
A couple of suggestions that leap out, based on a quick scan.
CAVEAT: This code was written in the browser and not compiled -- sorry for any errors, but you should get the idea.
1) The operation in SetClassificationIdentificationValues below seems like a common one.
I would encapsulate it like this:
let SetEntityReference (...
20
Paraphrasing Kent Beck (IIRC), I'd design my types so that concerns that change together are kept together, whereas concerns that change separately should be kept as separate as possible.
With that in mind, I'd start defining shopping basket items as simply as possible:
type Good = { SKU : string; Price : int }
This also enables you to define the price ...
11
The algorithm is overly complicated, and incorrect: for example, it will fail on the input (())().
I feel like it could ... use a more functional approach to solving the problem.
There is a more functional approach, maintaining a count of how many parentheses are open.
Let's start with the function signature
let rec isNested' (parens : char list) (...
11
What you have there is a value, but you probably want it to be a function:
let rec readlines () = seq {
let line = Console.ReadLine()
if line <> null then
yield line
yield! readlines ()
}
11
Even though the SOLID principles are known to be principles related to Object-Oriented Design, I'd still take a cue from the Dependency Inversion Principle. As APPP states it (ch. 11): "clients [...] own the abstract interfaces".
You can apply this principle in FP as well, in the sense that you can start by defining the overall behaviour of the function you'...
10
Looks good to me.
Couple of things to consider though:
You could split out two functions, squareOfSum and sumOfSquares. I think that would make it a little easier to read.
You could make the functions more general by using Seq.sum/Sum.sumBy instead of List.sum/List.sumBy.
While you could use fold, as joranvar mentioned, I think this would make the program ...
9
Some small comments
let tuple = handleChar c lines builder
let newLines = fst tuple
let newbuilder = snd tuple
getLines newLines newbuilder
can become
let newlines,newbuilder = handleChar c lines builder
getLines newLines newbuilder
although, if you aren't using getLines anywhere else, there is a good argument for making it
...
9
Changing the data structure
You're right to worry about the data structures. As Linus Torvals said:
git actually has a simple design, with stable and reasonably well-documented data structures. In fact, I'm a huge proponent of designing your code around the data, rather than the other way around, and I think it's one of the reasons git has been fairly ...
8
Mark's response does answer as to the warning you received.
An alternate approach could be
Seq.initInfinite // (1)
(fun _ -> Console.ReadLine())
|> Seq.takeWhile (fun line -> line <> null) // (2)
Edit: Could be restructured and more succinct as
fun _ -> Console.ReadLine()
|> Seq.initInfinite // (1)
|> Seq.takeWhile ((&...
8
You are correct in your statement that this is a pretty standard OOP implementation. The key signs are lots of mutable state and explicit loops. It's hard to give a line-by-line critique because a functional program would be structured so differently (maybe this is why nobody has posted an answer yet!)
First let's tackle a small stylistic point: It's ...
8
I actually wouldn't bother defining a bind2 as this can lead to an explosion, bind3, 4, 5, 6. Instead I would define a simple computation builder to do this. So using your code from above.
type DemoBuilder() =
member x.Bind(m,f) = bind f m
member x.Return(s) = Success s
let demo = DemoBuilder()
which you can then use like so
let DemoB x = ...
7
Functional-first style tends to prefer immutability over mutability, so the fact that you're using ref can sometimes signify that the code could be more idiomatic.
Recursive Solution
But how do you keep track of the state of your program without mutation? The traditional functional solution is to use recursion so that each step of the algorithm is isolated ...
7
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 ...
7
Some things:
let mutable itemCount = -1;
for _ in this.Items do
itemCount <- itemCount + 1
can become
let itemCount = this.Items |> List.length
In terms of types, in F# I would probably make Inventory Item a record rather than a class.
The easiest way to know if something needs to be mutable and you are stuck is to leave it off and ...
7
(Apologies in advance for any bad F# syntax in this answer)
One issue is that you're doing things statefully. In a functional style, you want functions to be pure, as much as possible- i.e. their purpose is to return something, not to change state. Whereas you have global collections like sieve and bigPrimes, and functions whose purpose is to modify those ...
7
Overall this looks ok, just a couple nitpicks:
In assertAreEqual, you're using string concatenation and then passing the resulting string to formatting function. You could use the formatting function to begin with:
if expected <> actual then
printfn "Test failed, expected %A, actual %A" expected actual
else
printfn "Test passed"
...
7
The solution should look like (pseudocode, I know little F# too ;) ):
String
|> ChunksOf 2
|> Map Reverse
|> Flatten
An example explains clearly
"abcd" -> ["ab", "cd"] -> ["ba", "dc"] -> "badc"
The key of effective Functional Programming is making good use of the built-in functions and carefully combining them.
7
One of the many benefits of F# is that it's a multiparadigmatic language; while it embraces a functional-first ideal, it clearly also enables you to write object-oriented code. This is useful if you're coming from a C# background, like I did some years ago. You can get started quickly writing F#, concentrating on learning the language syntax. Naturally, if ...
6
One way to work around invalid dates is not to generate them in the first place: find the number of days between the boundary dates, generate a random integer from this range and then add that number of days to the start date:
let generateDate (min : Date) max =
let totalDays = int (max - min).TotalDays
let generatedDays = rnd.Next(0, totalDays)
...
6
I like this! Your use of discriminated unions and active patterns is quite elegant, I think. (Although exploiting that compare on discriminated unions does what it does seems a bit of a hack–although I have a hard time justifying that sentiment.)
As to your concerns:
Here is a manual way to do it functionally (replace the let flag ... flag.Value with this):...
6
I don't really have any substantive comments about the code, just minor notes about the syntax.
It's just an opinion, but I think the custom !<> operator is kind of funky. An alternative approach would be to use a type extension to add methods to the XElement and XAttribute types:
type XContainer with
member this.Element(name) = this.Element(...
6
try ... with | _ -> false is bad. Like, very bad. The equivalent in C# is
try
{
...
}
catch (Exception)
{
return false;
}
See this answer on SO for good advice on exception handling, and this one line in particular:
Only catch what you can handle and recover from.
In that particular code, the only exception I can imagine being thrown is an ...
6
new() = new Item()
This is infinite recursion, in order to call the parameterless constructor, you call the parameterless constructor. It's not quite clear to me what the default values of id and name should be. In C#, it might be okay to use null, but that's not the common approach in F#.
query {
for row in db.ItemsTable do
select row
}
Wouldn't ...
6
All of the match expressions can be simplified. For example,
match fileExists nugetPackagesFile with
| t when t = true -> ...
| _ -> ...
Can be
match fileExists nugetPackagesFile with
| true -> ...
| _ -> ...
Similarly,
match appType with
| t when t = "web" -> ...
| t when t = "nuget" -> ...
| _ -> ...
Can be
match appType ...
6
I think that Mark's answer gives a great alternative perspective here.
That said, I would probably write something that's quite similar to what you did here. There is a couple of small tricks that I would perhaps use to make the code a bit nicer (at least for my own personal idea of "nicer" F# code :-)), so I thought I'd share those.
First of all, I would ...
6
A few points:
The value i on the second line is not used. It is customary to use underscore to name such values.
The expression cycle - cycle + n seems a bit redundant :-)
If you swap the last two arguments of height, you can use it in partially applied form as argument to List.map on the last line (i.e. List.map (height 1 0)), instead of introducing extra ...
6
Overall, I like the code. The types are defined nicely, and the organization makes sense to me. I'm still looking in changes that could work and benefit the code, but so far, I've come up with this:
Simplify some functions.
For subToZero, you match on a bool. This can be simpler if you just use an if expression: let subToZero a b = if (a >= b) then a - ...
6
If I understand correctly, you're looking for the Cartesian product of the attributes in each attribute category.
To get the Cartesian product, I've adapted* Eric Lippert's solution from his blog post Computing a Cartesian product with LINQ.
let cartesianProduct xs =
Seq.fold (fun acc xs -> seq {
for accSeq in acc do
for x in xs do
...
6
First off, I don't see a real need for these two variables:
let valueMod3 = value % 3
let valueMod5 = value % 5
To me, they just seem a little unnecessary. In fact, writing out the expression value % 3 or value % 5 takes the exact same amount of keystrokes as writing valueMod3 or valueMod5.
Next, I'd highly recommend that you get into the habit of using ...
6
After working for far too long on this, here is a review focusing mainly on performance issues, and possibly introducing some non functional stuff along the way. Lets take it problem by problem.
But let me start by stating that your code looks nice and clean, and without knowing any official style guides for F#, it is quite readable once you get a hang of ...
Only top voted, non community-wiki answers of a minimum length are eligible