UPGRADE YOUR SKILLS: Learn advanced Swift and SwiftUI on Hacking with Swift+! >>

Day 44 Challenge. Refactoring

Forums > 100 Days of Swift

Hello everyone, I completed the challenge, but it seems to me that my code looks bad.

What I don't like:

  1. I check if the user clicked on the cell before by comparing the name with the standard "Unknown", I think this is bad. If the default name is changed in the code, then the application will not work as it should. Is there any other way to track if the user has previously changed the name in a different way?

  2. I don't like that I have to write the same code several times, namely to create two UIAlertController and create and add UIAlertAction for each. I tried to write a showAlert function that I could call and change only the text, but I didn't succeed.

func showAlert(title: String, message: String, indexPath: IndexPath) {

        let person = people[indexPath.item]

        let ac = UIAlertController(title: title,
                                   message: message,
                                   preferredStyle: .alert)

        ac.addTextField()

        ac.addAction(UIAlertAction(title: "Cancel", style: .default))

        ac.addAction(UIAlertAction(title: "OK", style: .default) { [weak self, weak ac] _ in

            guard let newName = ac?.textFields?[0].text else { return }

            person.name = newName

            self?.collectionView.reloadData()
        })

        present(ac, animated: true)
    }
 func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) {

        let person = people[indexPath.item]

        if person.name == "Unknown" {

            showAlert(title: "Add username",
                      message: "", indexPath: indexPath)
        } else {

            let questionController = UIAlertController(title: "Delete user?",
                                                       message: "",
                                                       preferredStyle: .alert)

            questionController.addTextField()

            questionController.addAction(UIAlertAction(title: "Rename",
                                                       style: .default) { [weak self, weak questionController] _ in

                guard let newName = questionController?.textFields?[0].text else { return }

                person.name = newName

                self?.collectionView.reloadData()
            } )

            questionController.addAction(UIAlertAction(title: "Cancel",
                                                       style: .cancel))

            questionController.addAction(UIAlertAction(title: "Delete",
                                                       style: .default)  { _ in

                self.collectionView.deleteItems(at: [indexPath])

                self.people.remove(at: indexPath.item)
            })

            present(questionController, animated: true)
        }
    }
}

3      

I did mine like this...

override func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) {
        let person = people[indexPath.item]

        let changeNameAlertController = UIAlertController(title: "Change Name", message: nil, preferredStyle: .alert)

        changeNameAlertController.addTextField()

        changeNameAlertController.addAction(UIAlertAction(title: "Cancel", style: .cancel))

        changeNameAlertController.addAction(UIAlertAction(title: "OK", style: .default) { [weak self, weak changeNameAlertController] _ in
            guard let newName = changeNameAlertController?.textFields?[0].text else { return }
            person.name = newName
            self?.collectionView.reloadData()
        })

        let editOptionsAlertController = UIAlertController(title: "What would you like to do?", message: nil, preferredStyle: .actionSheet)

        editOptionsAlertController.addAction(UIAlertAction(title: "Change Name", style: .default) { [weak self] _ in
            self?.present(changeNameAlertController, animated: true)
        })

        editOptionsAlertController.addAction(UIAlertAction(title: "Delete", style: .destructive) { [weak self] _ in
            self?.people.remove(at: indexPath.item)
            self?.collectionView.reloadData()
        })

        editOptionsAlertController.addAction(UIAlertAction(title: "Cancel", style: .cancel))

        present(editOptionsAlertController, animated: true)
}

Mine doesn't check if they have changed the name before, because that shouldn't really matter. It still allows them to change the name again any time they click on the cell, even if they have already changed the name before.

It still involves creating 2 separate alert controllers. But I don't really think that is something that needs to be avoided.

I didn't use a showAlert() function at all. Just used the present() function to show the alert with the options in it, then had the alert's action call present again to show the alert where they could change the name.

3      

Although, looking at my code again now, this way is probably better...

override func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) {
        let editOptionsAlertController = UIAlertController(title: "What would you like to do?", message: nil, preferredStyle: .actionSheet)

        editOptionsAlertController.addAction(UIAlertAction(title: "Change Name", style: .default) { [weak self] _ in
            let changeNameAlertController = UIAlertController(title: "Enter New Name", message: nil, preferredStyle: .alert)
            changeNameAlertController.addTextField()
            changeNameAlertController.addAction(UIAlertAction(title: "Cancel", style: .cancel))

            changeNameAlertController.addAction(UIAlertAction(title: "OK", style: .default) { [weak changeNameAlertController] _ in
                let person = self?.people[indexPath.item]
                guard let newName = changeNameAlertController?.textFields?[0].text else { return }
                person?.name = newName
                self?.collectionView.reloadData()
            })

            self?.present(changeNameAlertController, animated: true)
        })

        editOptionsAlertController.addAction(UIAlertAction(title: "Delete", style: .destructive) { [weak self] _ in
            self?.people.remove(at: indexPath.item)
            self?.collectionView.reloadData()
        })

        editOptionsAlertController.addAction(UIAlertAction(title: "Cancel", style: .cancel))
        present(editOptionsAlertController, animated: true)
}

Then it only creates the changeNameAlertController when it is needed.

4      

BUILD THE ULTIMATE PORTFOLIO APP Most Swift tutorials help you solve one specific problem, but in my Ultimate Portfolio App series I show you how to get all the best practices into a single app: architecture, testing, performance, accessibility, localization, project organization, and so much more, all while building a SwiftUI app that works on iOS, macOS and watchOS.

Get it on Hacking with Swift+

Sponsor Hacking with Swift and reach the world's largest Swift community!

Archived topic

This topic has been closed due to inactivity, so you can't reply. Please create a new topic if you need to.

All interactions here are governed by our code of conduct.

 
Unknown user

You are not logged in

Log in or create account
 

Link copied to your pasteboard.