0
\$\begingroup\$

I have this code

@objc func didClickActionButton(sender: UITapGestureRecognizer) {

    if let contents = contentList {

      if contents[currentScreenIndex].customMeta["permission-location"] != nil {
        //request Permission
        ApiHelper.shared.initLocationHelperWithBeacon()
      }
      if contents[currentScreenIndex].customMeta["permission-notification"] != nil {
//        PushNotificationHelper.requestNotificationAuthorization { }
        PushNotificationHelper.requestNotificationAuthorization {
          DispatchQueue.main.async {
            if self.currentScreenIndex == contents.count - 1 {
                self.finishOnboarding()
              return
            }
           self.currentScreenIndex+=1
           self.showScreenAtPosition(position: self.currentScreenIndex)
          }
        }
        return
      }
      if currentScreenIndex == contents.count - 1 {
        finishOnboarding()
        return
      }
      currentScreenIndex+=1
      showScreenAtPosition(position: currentScreenIndex)
    }
  }

As you can see I have the same code in the DispatchQueue.main.async as I have at the bottom at the func. Is there anyway to refactor this so I don't write the same code twice? Is it even worth it to refactor this?

\$\endgroup\$
  • \$\begingroup\$ Use functions, pure ones, as much as you can. \$\endgroup\$ – ielyamani Feb 6 at 12:00
2
\$\begingroup\$

Refactor the repeated code into its own private function:

@objc func didClickActionButton(sender: UITapGestureRecognizer) {
    guard let contents = contentList else { return }

    if contents[currentScreenIndex].customMeta["permission-location"] != nil {
        //request Permission
        ApiHelper.shared.initLocationHelperWithBeacon()
    }

    if contents[currentScreenIndex].customMeta["permission-notification"] != nil {
        PushNotificationHelper.requestNotificationAuthorization {
            DispatchQueue.main.async {
                self.advanceOnboarding()
            }
        }
        return
    }

    advanceOnboarding()
}

private func advanceOnboarding() {
    if currentScreenIndex == contents.count - 1 {
        finishOnboarding()
        return
    }

    currentScreenIndex += 1
    showScreenAtPosition(position: currentScreenIndex)
}

I’d also suggest the guard for the early exit, as shown above.

Is it even worth it to refactor this?

Refactoring this code out into its own function not only avoids the unnecessary repetition (simplifying maintenance in the future), but it also makes this routine much easier to reason about. When first reading your code, it took a few seconds to figure out what you were trying to do, why that code was being repeated, etc. Giving that block of code a clear name makes it easier to grok what’s going on.

| improve this answer | |
\$\endgroup\$

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.