10
\$\begingroup\$

I've refactored my previous inventory system, and added a few features like removing items from the Inventory class, easily obtaining the current selected item through Inventory.GetSelectedItem, and some things have been renamed as well. My concerns are once again:

  • Is this the correct usage of F#'s type/class system? Should this be made in a more "functional" way?
  • Am I using getters/setters correctly? Do I need to declare the mutable variables internalName in my types, or is this automatically done?
  • Is my code properly styled?

Item.fs

namespace InventorySystem
    open System

    /// <summary>
    /// Represents an item. This type is only
    /// with the Inventory type.
    /// </summary>
    /// <param name="name">The item's name.</param>
    /// <param name="count">The amount of this specific item.</param>
    type Item(name: string, count: int) =
        let mutable _count = count
        let mutable _name = name

        member this.Name
            with get() = _name
            and set(value: string) = _name <- value

        member this.Count
            with get() = _count
            and set(value: int) = _count <- value

        /// <summary>
        /// Increment or decrement the how much of this
        /// specific item you have.
        /// </summary>
        /// <param name="amount">The amount to change by.</param>
        member this.ChangeCount(amount: int) =
            if this.Count + amount >= 0 then
                this.Count <- this.Count + amount
            else
                printfn "You cannot have less than zero items."

        /// <summary>
        /// Return this type in the following format:
        /// Name={item name}, Count={item count}.
        /// </summary>
        override this.ToString() =
            String.Format("Name={0}, Count={1}", this.Name, this.Count)

Inventory.fs

namespace InventorySystem
    open System

    /// <summary>
    /// This type represents an inventory, containing items.
    /// </summary>
    /// <param name="items">A list of items.</param>
    type Inventory(items: Item list) =
        let mutable _items = items
        let mutable _selectedItem = 0

        member this.Items
            with get() = _items
            and set(value: Item list) = _items <- value

        member this.SelectedItem
            with get() = _selectedItem
            and set(value: int) = _selectedItem <- value

        /// <summary>
        /// Add an item to the list of items.
        /// </summary>
        /// <param name="item">The item to add.</param>
        member this.AddItem(item: Item) =
            this.Items <- item :: this.Items

        /// <summary>
        /// Remove an item from the inventory based
        /// the item's index.
        /// </summary>
        /// <param name="itemIndex">The index of the item to remove.</param>
        member this.RemoveItem(itemIndex: int) =
            let mutable resultList = []

            for index = 0 to this.Items.Length - 1 do
                if index <> itemIndex then
                    resultList <- this.Items.[index] :: resultList

            this.Items <- resultList

        /// <summary>
        /// Change the currently selected item.
        /// </summary>
        /// <param name="amount">The amount to change the selected item by.</param>
        member this.ChangeSelectedItem(amount: int) =
            if this.SelectedItem + amount < this.Items.Length && this.SelectedItem + amount >= 0 then
                this.SelectedItem <- this.SelectedItem + amount
            else
                printfn "The currently selected item must stay in the range 0 -> Items.Length."

        /// <summary>
        /// Get the currently selected item.
        /// </summary>
        member this.GetSelectedItem() =
            this.Items.[this.SelectedItem]

        /// <summary>
        /// Return this type in the following format:
        /// Items={items}, Selected item={selected item}
        /// </summary>
        override this.ToString() =
            String.Format("Items={0}, Selected Item={1}", this.Items, this.Items.[this.SelectedItem])

Test.fs

open System
open InventorySystem

[<EntryPoint>]
let main argv =
    let myInventory = new Inventory([
        new Item("Sword of Death", 1);
        new Item("Gold", 1)
    ])

    myInventory.AddItem(new Item("Silver", 5))
    myInventory.AddItem(new Item("Copper", 10))
    Console.WriteLine(myInventory)

    myInventory.RemoveItem(1)
    Console.WriteLine(myInventory)

    Console.WriteLine(myInventory.GetSelectedItem())
    myInventory.ChangeSelectedItem(1)
    Console.WriteLine(myInventory.GetSelectedItem())

    Console.WriteLine(myInventory)
    Console.ReadKey()
    0
\$\endgroup\$
  • 1
    \$\begingroup\$ It's hard, at least for me, to suggest anything without a 'bigger picture', but I'd imagine the inventory to essentially be a map of item_id -> item. That's just my personal 'feeling', so take it with a huge grain of salt :) Oh, also the indexer on a linked list is O(n), as is any other index-based access, hence my reservations about the indexed approach overall... \$\endgroup\$ – Patryk Ćwiek Jul 26 '15 at 8:38
1
\$\begingroup\$

To answer your first question "Should this be made in a more "functional" way?"

  1. Embrace immutability.
  2. Prefer algebraic types and records over classes.

Here is my version:

type InventoryItem = {name:string}
type Inventory = Inventory of (InventoryItem * int) list

[<CompilationRepresentation (CompilationRepresentationFlags.ModuleSuffix)>]
module Inventory =

   let create items =
      Inventory items

   let countItem item (Inventory inventory) =
     inventory 
     |> List.tryFind (fun t -> fst t = item) 
     |> Option.bind (fun t -> Some (snd t))

   let add item count (Inventory inventory) =
     match countItem item (Inventory inventory) with
     |Some c -> create ((item,c + count)::(inventory |> List.filter (fun t -> fst t <> item)))
     |None -> create ((item,count)::inventory)

Inventory.create [{name="Sword of Death"},1;{name="Gold"},1]
  |> Inventory.add {name="Silver"} 1
  |> Inventory.add {name="Copper"} 5
  |> Inventory.add {name="Copper"} 5
  |> Inventory.add {name="Gold"} 2

Run in FSI the output is:

val it : Inventory = Inventory [({name = "Gold";}, 3); ({name = "Copper";}, 10); ({name = "Silver";}, 1); ({name = "Sword of Death";}, 1)]

\$\endgroup\$
4
\$\begingroup\$

There's a few small issues that I noticed with this.

The cons operator, ::, adds an item to the beginning of the list. I've done this in the Inventory.AddItem method. Since most people are used to items being added to the end of the list, the Inventory.AddItem method should be changed to this:

member this.AddItem(item: Item) =
    this.Items <- List.append [item] this.Items

Although, since List.append supports an entire 'T list as input, something like this can be done:

member this.AddItems(items: Item list) =
    this.Items <- List.append items this.Items

In addition to that, the Item.ChangeCount method is completely useless. I can just refactor Item.Count's setter to something like this:

member this.Count
    with get() = _count
    and set(value: int) =
        if this.Count + value >= 0 then
            this.Count <- this.Count + value
        else
            printfn "You cannot have less than zero items."
\$\endgroup\$
  • \$\begingroup\$ I'm trying to create this same thing. F# Inventory system without mutables. Any chance you can post your final code somewhere? I can ask a new question if you want. Thanks! \$\endgroup\$ – Anthony Nichols Feb 9 '16 at 16:48

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.