4
\$\begingroup\$

I am a newbie in F# and I am thinking about use F# in my next project. The project will work the database and I need to store and retrieve instances of objects in the Db.

Could you please check my code and say if there is something that I can improve?

Here is a simple object:

namespace Entities type Item(id : string, name : string) = member val ItemId = id with get, set member val ItemName = name with get, set new() = new Item() 

Here is Database Layer:

namespace SqlController open System open System.Data open System.Data.Linq open Microsoft.FSharp.Data.TypeProviders open Microsoft.FSharp.Linq open Entities type dbSchema = SqlDataConnection<"Data Source=VM-WIN8\SQLEXPRESS;Initial Catalog=mobile;Integrated Security=SSPI;"> type SqlConnector() = // universal sql functions member private this.DeleteRowsFrom (table : Table<_>) rows = table.DeleteAllOnSubmit(rows) // map functions member private this.Item2Record (item : Item) = new dbSchema.ServiceTypes.ItemsTable( ItemId = item.ItemId, ItemName = item.ItemName) member private this.Record2Item (e : dbSchema.ServiceTypes.ItemsTable) = new Item(e.ItemId, e.ItemName) // ### Items // return all items member this.GetItems() = use db = dbSchema.GetDataContext() query { for row in db.ItemsTable do select row } |> Seq.map this.Record2Item |> Seq.toArray // return one item by item id member this.GetItemByItemId(itemId : string) = use db = dbSchema.GetDataContext() query { for rows in db.ItemsTable do where (rows.ItemId = itemId) select rows } |> (fun s -> if Seq.isEmpty s then None else s |> Seq.head |> this.Record2Item |> Some) // insert new item member this.InsertItem (item : Item) = use db = dbSchema.GetDataContext() item |> this.Item2Record |> db.ItemsTable.InsertOnSubmit try db.DataContext.SubmitChanges() with | exn -> printfn "Exception: \n%s" exn.Message // update item member this.UpdateItem (item : Item) = use db = dbSchema.GetDataContext() query { for rows in db.ItemsTable do where (rows.ItemId = item.ItemId) select rows } |> Seq.nth 0 |> (fun e -> e.ItemName <- item.ItemName) try db.DataContext.SubmitChanges() with | exn -> printfn "Exception: \n%s" exn.Message // delete item by itemId member this.DeleteItemByItemId(itemId : string) = use db = dbSchema.GetDataContext() query { for rows in db.ItemsTable do where (rows.ItemId = itemId) select rows } |> this.DeleteRowsFrom db.ItemsTable try db.DataContext.SubmitChanges() with | exn -> printfn "Exception: \n%s" exn.Message 

Here is some tests:

module Tests open NUnit.Framework open FsUnit open Entities open SqlController [<TestFixture>] type SqlControllerTest() = let sqlConnector = new SqlConnector(); [<Test>] member x.SelectionAllTest() = let items = sqlConnector.GetItems() items.Length |> should equal 3 [<Test>] member x.SelectById() = let item = sqlConnector.GetItemByItemId("20") if Option.isSome item then (Option.get item).ItemName |> should equal "two" [<Test>] member x.SelectNotExistingItem() = let item = sqlConnector.GetItemByItemId("xx") Option.isNone item |> should equal true 
\$\endgroup\$
1
  • \$\begingroup\$ I have removed the second off-topic question instead of putting the entire question on hold. \$\endgroup\$ Commented Feb 22, 2015 at 9:48

1 Answer 1

6
\$\begingroup\$
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 just db.ItemsTable work here too?


query { for rows in db.ItemsTable do where (rows.ItemId = itemId) select rows } |> (fun s -> if Seq.isEmpty s then None else s |> Seq.head |> this.Record2Item |> Some) 

That second part could be simplified to:

|> Seq.tryPick Some |> Option.map this.Record2Item 

try db.DataContext.SubmitChanges() with | exn -> printfn "Exception: \n%s" exn.Message 

Are you sure that it's okay for your DB layer to just print any errors to the console and then continue as if everything was fine? That doesn't sound like a very robust approach to me.

If you're sure you want to do it this way, then consider extracting this code into a separate function, since you're repeating it at several places.


|> (fun e -> e.ItemName <- item.ItemName) 

I personally don't like this style of code. Pipelining using |> should be used for code that doesn't mutate anything.


if Option.isSome item then (Option.get item).ItemName |> should equal "two" 

I would probably use pattern matching here.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.