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) (...
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
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 ()
}
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
Second revision
Ok, first of all it would really help if you added some comments. I would also suggest giving your variables names which have more than one letter.
In particular it would be a good idea to describe the algorithms being used. In particular you should document what kind of tree and balancing algorithm you're using.
Speaking of: your tree ...
8
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
...
8
(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 ...
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
This is a bit difficult to answer, because you did not share code sample that fully type-checks (and so it is hard to make sure the answer is correct). However, I think you're very close. When refactoring two similar functions, you just need to abstract out the bits that differ.
In your case, there are two places:
One is the function that is used to ...
7
This isn't really F# specific because you could do the same thing in your C# code, but converting each character to a string so you can parse it as an integer seems inefficient to me. I'd be inclined to use Char.GetNumericValue instead.
Also, you could eliminate a step in your method chain by using Seq.sumBy instead of Seq.map + Seq.sum.
I might write your ...
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
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'...
7
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 ...
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
Look into Array.choose. Using that function cuts the length of your code in half, because you have basically reinvented it. (I did the same at some point ;-)
6
Most of your code can be replaced with a comprehension:
type Comment = { Author: string; Body: string }
let readAllComments () =
[|for line in System.IO.File.ReadAllLines("comments.txt") do
match line.Split ';' with
| [|author; body|] -> yield {Author=author; Body=body}
| _ -> ()|]
6
You should indent the body of the qarray function to make it more obvious where it starts and where it's end (I mean I know it ends at the end of the file in this case, but that's not immediately apparent at first glance).
You should also rename your function f into something more descriptive.
let tableData = File.ReadAllLines tableName
If you're using ....
6
Apparently there is a strong opinion that this is not a good question for SO, hence no good answer may be given. Nevertheless, I'll try to approach it from efficiency side.
Regardless of quality of given implementation of Dijkstra's algorithm it may be not a good approach for solving Project Euler Problem 81. The latter has specifics allowing for much ...
6
Your code formatting is very readable. The minor issue is that you don't have to indent after namespace declaration. I would choose to open module directly to save a level of indentation (see attached code later).
So what are the best practices for code format and file layout?
I think F# code formatting guidelines have very concrete suggestions on these ...
6
I general, I agree with what mavnn said in his answer, I just think the function can be written in a simpler way:
let rec performOptimizations ast =
let optimized = optimizeAst ast
if ast = optimized then
ast
else
performOptimizations optimized
This method also sounds like a good candidate for making it more generic, something ...
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(...
Only top voted, non community-wiki answers of a minimum length are eligible
