Error while creating a closure inside a function

Question

Thanks a lot for the reply. It's true that I am learning to write closures at this stage. Actually I read somewhere about the "forEach" function that works on collections and that takes a single argument as parameter (i.e a closure).

Syntax of forEach is "Void forEach(body: (Int) throws -> Void) rethrows"

What I am trying to do is write a similar function with a single parameter (i.e a closure) that will calculate the Factorial of a number and so we can print the Factorial of that number. I don't want to pass that number as a second parameter to that function.

I understand that forEach is a member function of Collections class that works on each element one by one. So it picks up the elements from within the array. Similarly I have created a private property (factorialNumber) inside my class (whose value I can set using the public function "setFactorialNumber"). Now I am trying to create a public function (factorial) for my class that will have only one parameter (i.e the closure) that will use the value of "factorialNumber" property internally and calculate the factorial of that number that we can print from outside when we call that function from the other code.

Below is my class..

public class MyArray {
    private var factorialNumber = 0

    public func setFactorialNumber(factorialNumber value: Int) {
        factorialNumber = value
    }

    public func factorial(body closure: (Int) -> Void) -> Void {
        var outputString: String?
        var result = 1

        if factorialNumber <= 0 {
            outputString = nil
        } else {
            outputString = ""
            while(factorialNumber >= 1) {
                if factorialNumber == 1 {
                    outputString = outputString! +  "\(factorialNumber) = \(result)"
                    break
                } else {
                    outputString = outputString! + "\(factorialNumber) x "
                }
                result = result * factorialNumber
                factorialNumber -= 1
            }
        }
    }
}

Your question indicates little to no knowledge of closures. Please read up on the concept and understand the purpose of closures. From what I can see in your question, you only need a function (no closures). Closures serve a different purpose to what you seem to be trying to achieve — Malik
– Malik, Commented May 30, 2018 at 5:04
Not related to programming, but if you plan on using this for actual math you should note that 0 is also a valid input (0! = 1) — John Montgomery
– John Montgomery, Commented Jun 1, 2018 at 0:48

staticVoidMan · Accepted Answer · 2018-06-01 05:21:00Z

1

A closure is basically a function that wants to be called.
In your case, you're defining it but you're not using it.

Problems I see in factorial(body:):

You should execute the closure atleast once.
i.e. do the following at some point/s:
```
closure(someValue)
```
You seem to maintain an outputString, which looks like the thing you want the closure to take.
In this case, your closure should take a String instead of an Int.
```
func factorial(body closure: (String?) -> Void) -> Void { //...
```

Finally, this is what the function should look like:

func factorial(body closure: (String?) -> Void) -> Void {
    var outputString: String?

    //...your factorial logic as:
    if factorialNumber <= 0 {
        outputString = nil
    } else {
        outputString = ""
        while(factorialNumber >= 1) {
            if factorialNumber == 1 {
                outputString = outputString! +  "\(factorialNumber) = \(result)"
                break
            } else {
                outputString = outputString! + "\(factorialNumber) x "
            }
            result = result * factorialNumber
            factorialNumber -= 1
        }
    }

    //Finally, the closure call
    closure(outputString)
}

And looking at your structure, it's usage will be:

let factorial = MyArray()
factorial.setFactorialNumber(factorialNumber: 20)
factorial.factorial { (result) in
    print(result)
}

It's output is:

20 x 19 x 18 x 17 x 16 x 15 x 14 x 13 x 12 x 11 x 10 x 9 x 8 x 7 x 6 x 5 x 4 x 3 x 2 x 1 = 2432902008176640000

NOTE: Your class is Crashable

The answer above is sufficient for the particular context of your question but duly note:

Your logic uses Int, so numbers higher than 20 cause your code to crash

Solution:
- Use Double
- Guard your logic from crashing on inputs too large to handle
If you don't setFactorialNumber(factorialNumber:), and directly jump to calling factorial(body:), your code will crash.

Improvement?:

Change classname from MyArray to FactorialFinder or something

Add an init to this class:

init(with number: Int) {
    self.factorialNumber = number
}

Now that we have a custom init, make the default init private:
```
private init() {}
```
- So something like this is not possible FactorialFinder()
- And FactorialFinder(with:) becomes the only way to initialize this class

edited Jun 1, 2018 at 5:21

answered Jun 1, 2018 at 0:36

staticVoidMan

20.4k6 gold badges76 silver badges104 bronze badges

Sign up to request clarification or add additional context in comments.

11 Comments

Vermaji Over a year ago

Than you sooo much for the help buddy. I tried the solution, but the output that I get in the playground is: () arr1.setFactorialNumber(number: 5) arr1.factorial{ (result) in print(result ?? "Result is Nil") } Output: ()

Vermaji Over a year ago

Thank you soo much for the reply. I have used the solution that you provided. But the output that I get in playground is:

staticVoidMan Over a year ago

@vermau Check again. I confirmed it in playground before posting an answer. You may have missed something. Btw, I've updated answer including your factorial logic.

Vermaji Over a year ago

Hello dear, I have uploaded my file on dropbox and here is the link. Can you please see what's going wrong. It is a very small file. Thanks in anticipation. dropbox.com/s/tctncxbx3nbq57z/Factorial.zip?dl=0

staticVoidMan Over a year ago

@vermau I ran your playground setup and it's output is 5 x 4 x 3 x 2 x 1 = 120. Looks fine to me except for all those arr1.insert(value:) that don't really make sense to me.

|

zneak · Accepted Answer · 2018-05-30 01:56:57Z

0

Your first example is a good hint of your problem:

factorial {
 print("Factorial is: \($0)")
}

What is the exact text that you want this function to print? "Factorial is: 100", "Factorial is: 200"?

This is a trick question, because the answer is unknowable: your function doesn't accept a number to calculate the factorial from, it only accepts a trailing closure that defines what to do with the result.

answered May 30, 2018 at 1:56

zneak

139k48 gold badges270 silver badges342 bronze badges

3 Comments

Vermaji Over a year ago

Hi, Thanks a lot for the reply. It's true that I am learning to write closures at this stage. Actually I read somewhere about the "forEach" function that works on collections and that takes a single argument as parameter (i.e a closure).Syntax of forEach is "Void forEach(body: (Int) throws -> Void) rethrows"

zneak Over a year ago

Yes, but forEach is an array method, which allows it to operate on the contents of the array. It's array.forEach { print($0) }, where the closure body is repeated with each element of array; not forEach { print($0) }, which doesn't have anything that it can obviously operate on.

Vermaji Over a year ago

Hi you are so prompt.. amazing. I have just added my whole class code in the section above and also explained what I am trying to achieve. Please see if you can help me resolve my problem. I have been trying to solve this for two days now

Collectives™ on Stack Overflow

Error while creating a closure inside a function

2 Answers 2

NOTE: Your class is Crashable

Improvement?:

11 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

NOTE: Your class is Crashable

Improvement?:

11 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related