current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

Trying to simply my code below:

for (index, point) in enumerate(self.points){
        let pointButton = UIButton()
        pointButton.frame = point.bounds
        self.view.addSubview(pointButton)
    }

Is there a better way to accomplish this through .map .filter or .reduce ?

share|improve this question
1  
you could, but honestly, having self.view.addSubview() removes any point of doing so because that is a side-effect. I don't know enough Swift to write this out, but you would want to use map over the self.points collection and have it return your pointButton, and then the mapped array as subviews. –  A Red Herring Jul 10 at 23:59
    
Thanks. Can you please elaborate on the side-effect? –  user1107173 Jul 11 at 0:01
1  
I'll try; basically, in FP, you generally strive to create 'pure functions' - they only operate on the arguments they are passed in, only interact with other pure functions, always return a value and they don't modify existing values/outside world. By having self.view.addSubview() in the function, you are causing a side effect because you are modifying the outside world (self.view.addSubview() stores state). Obviously every useful program uses state, but using map "just because" doesn't make it FP because of the reasons I've mentioned. –  A Red Herring Jul 11 at 0:04
1  
In reality there's likely a nicer way to write this using map, but you're going to have to separate this out into multiple functions and I just don't know enough swift (any) to do that. Maybe someone else will, I think we have a few swift users here –  A Red Herring Jul 11 at 0:09
1  
Why are you using enumerateif you're not using the index? –  Veedrac Jul 11 at 0:59

1 Answer 1

up vote 3 down vote accepted

The index is not used at all, therefore you do not need enumerate():

for point in self.points { ... }

(You mentioned in a comment that the index is used somewhere else, but we can review only the code that is shown in your question.)

let pointButton = UIButton()
pointButton.frame = point.bounds

can be simplified to

let pointButton = UIButton(frame: point.bounds)

and then you could get rid of the intermediate variable:

for point in self.points {
    self.view.addSubview(UIButton(frame: point.bounds))
}

and this looks like the best solution in your case to me.

You could use map() for that purpose:

self.points.map {
    self.view.addSubview(UIButton(frame: $0.bounds))
}

But using map() for its side effects is frowned-upon, see for example Higher order function: “Cannot invoke 'map' with an argument list of type '((_) -> _)'”, where an explicit foreach method is offered as an alternative:

extension Array {
    func foreach(function: T -> ()) {
        for elem in self {
            function(elem)
        }
    }
}

which can be used as

self.points.foreach {
    self.view.addSubview(UIButton(frame: $0.bounds))
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.