3
\$\begingroup\$

I wrote a piece of code that creates an array of objects based on the array of dictionaries. The problem is that my solution uses multiple loops imperatively.

Let's consider the following case. There is a class called Drug:

class Drug {

var name:String
var dosage:[(dose:String,time:String)]

init(name:String){

    self.name = name
    self.dosage = []
    }
}

I obtain the data as an array of dictionaries:

var array = [["drugName":"Amotaks", "time":"17.00", "dose":"5"], ["drugName":"Amotaks", "time":"18.00", "dose":"5"], ["drugName":"Amotaks", "time":"18.00", "dose":"5"], ["drugName":"Amotaks", "time":"17.00", "dose":"5"], ["drugName":"Claritin", "time":"17.00", "dose":"5"], ["drugName":"Claritin", "time":"18.00", "dose":"5"]]

Now is the tricky part. What is the best way to turn this data into an array of Drug objects? I am not happy with my solution because it uses nested loops and additional helper arrays thus it's very inefficient.

The purpose is to create objects without repeating their names and provide each object with an dosage array which consist of (dose, time) tuples):

var DrugArray:[Drug] = []
    var drugs:[String] = []

    for i in array

        {
        if contains(drugs, i["drugName"]! as String){
            continue
            }
        else{

            drugs.append(i["drugName"]!)
            DrugArray.append(Drug(name: i["drugName"]!))

            }

        }

    for a in DrugArray{

        for i in array {
            if i["drugName"]! == a.name
            {
                a.dosage.append(dose: i["dose"]!, time: i["time"]!)

            }

        }}

I believe there is a better way to do that, probably in a few lines of code. I tried to figure out something with basic FP functions such as map or filter but I end up with nothing.

How can I perform the operation in an elegant way (probably using more advanced functional programming)?

\$\endgroup\$
  • \$\begingroup\$ Swift 1.2 or Swift 2.0? \$\endgroup\$ – nhgrif Jul 22 '15 at 16:06
  • \$\begingroup\$ swift 1.2 but swift 2.0 would be interesting as well \$\endgroup\$ – DCDC Jul 22 '15 at 16:07
5
\$\begingroup\$
var DrugArray:[Drug] = []
var drugs:[String] = []

for i in array {
    if contains(drugs, i["drugName"]! as String) {
        continue
    } else {
        drugs.append(i["drugName"]!)
        DrugArray.append(Drug(name: i["drugName"]!))
    }
}

for a in DrugArray {
    for i in array {
        if i["drugName"]! == a.name {
            a.dosage.append(dose: i["dose"]!, time: i["time"]!)
        }
    }
}

So, now that we've cleaned up the code and made it more readable, let's talk about some refactoring we can do.

The first thing I notice is that your nested loop at the bottom is entirely unnecessary. We can do all of that logic in the top loop.

We're also doing a lot of forced unwrapped... which you really don't want to do. All this is going to do is result in a crash in your app eventually.

We're also using single letter variable names which invokes no meaning to the reader. These loops are short... for now. If & when the contents of the loop grows (the top loop is already close to being at the point), you'll start to see the value in having meaningful variable names. It helps you remember exactly what the variable represents. Better variable names is just one part of making your code self-documenting.

And ultimately, as Mario Zahone's answer points out, what we clearly need is a dictionary rather than an array (and a separate array to keep track of what's already been tracked).

Finally, repeatedly appending to a collection almost always results in poor performance. If there's not enough space in the collection's current memory location for one more item, the entire collection has to be copied into a new memory location, which is an expensive task. In a loop with lots of iterations, this is likely to happen multiple times. So, when we can guess at a good minimum size, we want to go ahead and make sure our collection is allocated in a large enough memory space.

So, taking all of these things into account, the final implementation of our code probably looks something like this:

var drugs = [String:Drug](minimumCapacity: array.count)

for drugDictionary in array {        
    if let
        drugName = drugDictionary["drugName"],
        drugDose = drugDictionary["dose"],
        drugTime = drugDictionary["time"] {

        drugs[drugName] = drugs[drugName] ?? Drug(name: drugName)
        drugs[drugName]?.dosage.append(dose: drugDose, time: drugTime)
    }
}

If we really want an array, we can always fetch one from the dictionary:

let drugArray = drugs.values
\$\endgroup\$
  • \$\begingroup\$ Alright, works perfectly fine now, thanks:) However, you must admit that this is tricky that with wrong "name" key it resulted with 0 length drugs.count but with some existing drugArray. Why is that? \$\endgroup\$ – DCDC Jul 23 '15 at 12:44
  • \$\begingroup\$ The drugArray had 0 values in it. That comes from the drugs dictionary which we created, but has zero values because our key was wrong and we had zero values entered into it. \$\endgroup\$ – nhgrif Jul 23 '15 at 12:45
  • \$\begingroup\$ Ok I get it:) Actually I discovered one problem right now - this all works perfectly in playground, but when I apply this code to a real program, in which array is declared empty and filled with data after the download task, this is not going to work... \$\endgroup\$ – DCDC Jul 23 '15 at 13:08
  • \$\begingroup\$ So i initialized array as empty and of type [[String:String]], think this will work \$\endgroup\$ – DCDC Jul 23 '15 at 13:22
4
\$\begingroup\$
var DrugArray:[Drug] = []
    var drugs:[String] = []

    for i in array

        {
        if contains(drugs, i["drugName"]! as String){
            continue
            }
        else{

            drugs.append(i["drugName"]!)
            DrugArray.append(Drug(name: i["drugName"]!))

            }

        }

    for a in DrugArray{

        for i in array {
            if i["drugName"]! == a.name
            {
                a.dosage.append(dose: i["dose"]!, time: i["time"]!)

            }

        }}

So... I'm not going to bother about refactoring anything for right now. Your code has some just very basic formatting issues that deserve immediate attention.

Learning how to write this code in a leaner way is a matter of getting experience with language and coding in general. It can take time.

Learning how to format this properly? Well, if you're writing in Xcode, Xcode does basically all of the formatting for you, so you kind of have to fight it to get it in this sort of shape.

So, without further adieu...

var DrugArray:[Drug] = []
var drugs:[String] = []

for i in array {
    if contains(drugs, i["drugName"]! as String) {
        continue
    } else {
        drugs.append(i["drugName"]!)
        DrugArray.append(Drug(name: i["drugName"]!))
    }
}

for a in DrugArray {
    for i in array {
        if i["drugName"]! == a.name {
            a.dosage.append(dose: i["dose"]!, time: i["time"]!)
        }
    }
}

I've changed nothing but white space, and instantly the code is drastically more readable. We can actually make sense of what is going on now.

\$\endgroup\$
1
\$\begingroup\$

This is not purely functional, but might be worth a look:

var buckets: [String : Drug] = [:]
for d in array {
    if let name = d["drugName"], let dose = d["dose"], let time = d["time"] {
        let drug = buckets[name] ?? Drug(name: name)
        let dosage_item = (dose: dose, time: time)
        drug.dosage.append(dosage_item)
        buckets[name] = drug
    }
}
let result = buckets.values
\$\endgroup\$
  • 3
    \$\begingroup\$ Would you please elaborate on how this is an improvement? The educational value of this answer, as it is, seems pretty low. I would encourage you to read some other answers on the site to get a better grasp of what kind of answers make good Code Reviews. \$\endgroup\$ – Phrancis Jul 22 '15 at 23:55
  • 1
    \$\begingroup\$ @Phrancis. Thanks! I promise I will next time. For this answer is not needed anymore, since nhgrif already did a stellar job here \$\endgroup\$ – Mario Zannone Jul 23 '15 at 7:57

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.