passing data between vcs using delegate and protocol #13
Conversation
|
|
||
| import UIKit | ||
|
|
||
| protocol canRecieve { |
There was a problem hiding this comment.
Names of protocols, classes, structs, etc should all be capitalised. The name should also be descriptive, and spelled correctly (receive).
There was a problem hiding this comment.
I would add that even then I find this name wrong. The typical naming convention for something like this is {name of the VC}Delegate. In this case it would be: protocol JimmySecondViewControllerDelegate: class {
While delegates use protocols they have a specific use case and conventions. When you work in teams following platform and community conventions is super important.
|
|
||
|
|
||
| @IBAction func passItBack(_ sender: Any) { | ||
| delegate?.dataRecieved(data: textField.text!) |
There was a problem hiding this comment.
You should guard that the text field actually contains text, rather than force unwrapping.
There was a problem hiding this comment.
Indeed, there's very few instances where it does make sense to force unwrap this is not one of them.
|
|
||
| @IBAction func passItBack(_ sender: Any) { | ||
| delegate?.dataRecieved(data: textField.text!) | ||
| self.navigationController?.popViewController(animated: true) |
There was a problem hiding this comment.
Agreed. You only need to use self when dealing with a closure (for example in a closure after a WebService response comes in.
|
|
||
| var data = "" | ||
|
|
||
| @IBOutlet var label: UILabel! |
There was a problem hiding this comment.
IBOutlets should be declared as private (we don't want outsiders to modify them) and weak.
There was a problem hiding this comment.
Private makes sense. @ivancr regarding this, should IBOutlets be weak or strong, and why?
There was a problem hiding this comment.
Technically VC is not the owner of the label, the VC's view is and the VC already has a strong reference to its view.
If the VC also kept strong references to the view's children (e.g. this label) then those won't be deallocated if the view was to get swapped out. There may be use cases where you'd want a strong outlet, but I haven't run into one yet. YMMV 😃
There was a problem hiding this comment.
Apple recommends that in the ARC age your IBOutlets should be strong:
https://stackoverflow.com/questions/7678469/should-iboutlets-be-strong-or-weak-under-arc
There was a problem hiding this comment.
@ivancr i watched that session and was curious as to why they said that, as their documentation, Xcode defaults and sample code reflects the opposite.
I like this roundup: https://cocoacasts.com/should-outlets-be-weak-or-strong/
TLDR: It doesn't really matter — just be consistent.
| @IBAction func passItBack(_ sender: Any) { | ||
| delegate?.dataRecieved(data: textField.text!) | ||
| self.navigationController?.popViewController(animated: true) | ||
| // dismiss(animated: true, completion: nil) |
There was a problem hiding this comment.
Code that isn't necessary should be removed to avoid confusion.
There was a problem hiding this comment.
@jimmygtz15 Instead of commenting, can just delete the commented code.
|
|
||
| if indexPath.row == 10 { | ||
| let storyboard = UIStoryboard(name: "Jimmy", bundle: nil) | ||
| let destination = storyboard.instantiateViewController(withIdentifier: "JimmyViewController") as! JimmyViewController |
There was a problem hiding this comment.
Instead of the if statement, you can attach the if statement action to the switch statement and then return, example:
`case 10:
let storyboard = UIStoryboard...
return`
There was a problem hiding this comment.
Don't use a storyboard. Another important lesson to learn here is that you're working on a codebase with a team and you should conform to the style and architecture of it. Nobody else is using storyboards and you shouldn't either.
Be a nice team citizen and conform to the style or convince the team that we should change the style. Don't go rogue though.
| <fontDescription key="fontDescription" type="system" pointSize="25"/> | ||
| <textInputTraits key="textInputTraits"/> | ||
| </textField> | ||
| <label opaque="NO" userInteractionEnabled="NO" contentMode="left" horizontalHuggingPriority="251" verticalHuggingPriority="251" text="Label" textAlignment="natural" lineBreakMode="tailTruncation" baselineAdjustment="alignBaselines" adjustsFontSizeToFit="NO" translatesAutoresizingMaskIntoConstraints="NO" id="ASx-aW-Sso"> |
There was a problem hiding this comment.
You may want to add leading/trailing constraints to avoid the text from disappearing off screen if the label contents are very long.
There was a problem hiding this comment.
Give it some constraints
There was a problem hiding this comment.
Or better yet, get rid of the storyboard and do it programmatically. You can pair with @gionoa and he'll show you the ways of the force.
|
|
||
| var delegate : canRecieve? | ||
|
|
||
| var data = "" |
There was a problem hiding this comment.
Renaming to initialData may make it a little clearer what the role of this variable is.
There was a problem hiding this comment.
Always important for names to make sense.
There was a problem hiding this comment.
I would also go further and add that using the noun data makes this too generic. This is text we're talking here, it's a very specific type of data. The name should reflect not only in its use case but also its identity.
| import UIKit | ||
|
|
||
| protocol canRecieve { | ||
| func dataRecieved(data: String) |
There was a problem hiding this comment.
The function signature should also include the sender, so the delegate knows who called it:
func dataRecieved(from viewController: UIViewController, data: String)
There was a problem hiding this comment.
I know that you (Jimmy) were trying to follow the protocol pattern of a capability but I think you should scrap this idea and think of it as a Delegate specific to JimmySecondViewController. Then the function should be a specific need of JimmySecondViewController to communicate to another object. Maybe func didEnterText(_ text: String).
| } | ||
|
|
||
|
|
||
| @IBAction func passItBack(_ sender: Any) { |
There was a problem hiding this comment.
While this pattern works, it assumes that the previous view controller is the same as its delegate. A slightly better pattern may be to send the data back to the delegate and then allow the delegate to control any navigation (in this case popping the VC).
However, a much better approach is to use unwind segues here as they handle all of this for you, without the need of a delegate protocol or handling the navigation yourself. This is a great tutorial on this type of segue: https://matteomanferdini.com/unwind-segue/
There was a problem hiding this comment.
Didn't know that! Awesome!
There was a problem hiding this comment.
I'm sure there's somebody out there using these and they have their merits but I've never seen or work in any team that used them. I'd highly recommend you go the Delegate pattern.
Also, each part of the whole system should be named accordingly. This IBAction maybe happens when you tap a button, then make the name say that. What you do inside is another story for me.
|
|
||
| import UIKit | ||
|
|
||
| protocol canRecieve { |
There was a problem hiding this comment.
I would add that even then I find this name wrong. The typical naming convention for something like this is {name of the VC}Delegate. In this case it would be: protocol JimmySecondViewControllerDelegate: class {
While delegates use protocols they have a specific use case and conventions. When you work in teams following platform and community conventions is super important.
| import UIKit | ||
|
|
||
| protocol canRecieve { | ||
| func dataRecieved(data: String) |
There was a problem hiding this comment.
I know that you (Jimmy) were trying to follow the protocol pattern of a capability but I think you should scrap this idea and think of it as a Delegate specific to JimmySecondViewController. Then the function should be a specific need of JimmySecondViewController to communicate to another object. Maybe func didEnterText(_ text: String).
|
|
||
| class JimmySecondViewController: UIViewController { | ||
|
|
||
| var delegate : canRecieve? |
There was a problem hiding this comment.
Let me introduce you to memory leaks. Let me know if you see why you have memory leak here and how you'd fix it.
Also the space should only go after the :. Like this:
| var delegate : canRecieve? | |
| var delegate: canRecieve? |
|
|
||
| var delegate : canRecieve? | ||
|
|
||
| var data = "" |
There was a problem hiding this comment.
I would also go further and add that using the noun data makes this too generic. This is text we're talking here, it's a very specific type of data. The name should reflect not only in its use case but also its identity.
|
|
||
| var data = "" | ||
|
|
||
| @IBOutlet var label: UILabel! |
There was a problem hiding this comment.
Apple recommends that in the ARC age your IBOutlets should be strong:
https://stackoverflow.com/questions/7678469/should-iboutlets-be-strong-or-weak-under-arc
|
|
||
| } | ||
|
|
||
|
|
|
|
||
|
|
||
| @IBAction func buttonPressed(_ sender: Any) { | ||
| performSegue(withIdentifier: "sendDataForwards", sender: self) |
There was a problem hiding this comment.
Or just don't use segues, they're gross.
|
|
||
| let secondVC = segue.destination as! JimmySecondViewController | ||
|
|
||
| secondVC.data = textField.text! |
There was a problem hiding this comment.
If JimmySecondViewController requires a string to be used, then it should initialized with it.
| } | ||
| } | ||
|
|
||
| func dataRecieved(data: String) { |
There was a problem hiding this comment.
Agreed, this is pretty much a community standard. It should go at the end of the VC something like this:
extension JimmyViewController: JimmySecondViewController {
}
|
|
||
| if indexPath.row == 10 { | ||
| let storyboard = UIStoryboard(name: "Jimmy", bundle: nil) | ||
| let destination = storyboard.instantiateViewController(withIdentifier: "JimmyViewController") as! JimmyViewController |
There was a problem hiding this comment.
Don't use a storyboard. Another important lesson to learn here is that you're working on a codebase with a team and you should conform to the style and architecture of it. Nobody else is using storyboards and you shouldn't either.
Be a nice team citizen and conform to the style or convince the team that we should change the style. Don't go rogue though.
|
Thank you for taking the time to review this and providing your input.🙏
I'll send my update when I'm ready.
…On Thu, May 30, 2019 at 10:21 AM Paulo Fierro ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmySecondViewController.swift
<#13 (comment)>
:
> @@ -0,0 +1,41 @@
+//
+// JimmySecondViewController.swift
+// BackToBasicsUIKit
+//
+// Created by Jimmy Gutierrez on 5/23/19.
+// Copyright © 2019 SwiftMiami. All rights reserved.
+//
+
+import UIKit
+
+protocol canRecieve {
Names of protocols, classes, structs, etc should all be capitalised. The
name should also be descriptive, and spelled correctly (receive).
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmySecondViewController.swift
<#13 (comment)>
:
> + var delegate : canRecieve?
+
+ var data = ""
+
+ @IBOutlet var label: UILabel!
+ @IBOutlet var textField: UITextField!
+
+ override func viewDidLoad() {
+ super.viewDidLoad()
+ label.text = data
+
+ }
+
+
+ @IBAction func passItBack(_ sender: Any) {
+ delegate?.dataRecieved(data: textField.text!)
You should guard that the text field actually contains text, rather than
force unwrapping.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmySecondViewController.swift
<#13 (comment)>
:
> +
+ var data = ""
+
+ @IBOutlet var label: UILabel!
+ @IBOutlet var textField: UITextField!
+
+ override func viewDidLoad() {
+ super.viewDidLoad()
+ label.text = data
+
+ }
+
+
+ @IBAction func passItBack(_ sender: Any) {
+ delegate?.dataRecieved(data: textField.text!)
+ self.navigationController?.popViewController(animated: true)
self isn't necessary.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmySecondViewController.swift
<#13 (comment)>
:
> +// Copyright © 2019 SwiftMiami. All rights reserved.
+//
+
+import UIKit
+
+protocol canRecieve {
+ func dataRecieved(data: String)
+}
+
+class JimmySecondViewController: UIViewController {
+
+ var delegate : canRecieve?
+
+ var data = ""
+
+ @IBOutlet var label: UILabel!
IBOutlets should be declared as private (we don't want outsiders to
modify them) and weak.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmySecondViewController.swift
<#13 (comment)>
:
> + var data = ""
+
+ @IBOutlet var label: UILabel!
+ @IBOutlet var textField: UITextField!
+
+ override func viewDidLoad() {
+ super.viewDidLoad()
+ label.text = data
+
+ }
+
+
+ @IBAction func passItBack(_ sender: Any) {
+ delegate?.dataRecieved(data: textField.text!)
+ self.navigationController?.popViewController(animated: true)
+// dismiss(animated: true, completion: nil)
Code that isn't necessary should be removed to avoid confusion.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmyViewController.swift
<#13 (comment)>
:
> @@ -0,0 +1,42 @@
+//
+// JimmyViewController.swift
+// BackToBasicsUIKit
+//
+// Created by Jimmy Gutierrez on 5/23/19.
+// Copyright © 2019 SwiftMiami. All rights reserved.
+//
+
+import UIKit
+
+class JimmyViewController: UIViewController, canRecieve {
+
+ @IBOutlet var label: UILabel!
IBOutlets should be declared as private (we don't want outsiders to modify
them) and weak.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmyViewController.swift
<#13 (comment)>
:
> +//
+// JimmyViewController.swift
+// BackToBasicsUIKit
+//
+// Created by Jimmy Gutierrez on 5/23/19.
+// Copyright © 2019 SwiftMiami. All rights reserved.
+//
+
+import UIKit
+
+class JimmyViewController: UIViewController, canRecieve {
+
+ @IBOutlet var label: UILabel!
+ @IBOutlet var textField: UITextField!
+
+ override func viewDidLoad() {
If we're not overriding, we should remove the functionalists.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmyViewController.swift
<#13 (comment)>
:
> +
+import UIKit
+
+class JimmyViewController: UIViewController, canRecieve {
+
+ @IBOutlet var label: UILabel!
+ @IBOutlet var textField: UITextField!
+
+ override func viewDidLoad() {
+
+
+ }
+
+
+ @IBAction func buttonPressed(_ sender: Any) {
+ performSegue(withIdentifier: "sendDataForwards", sender: self)
Instead of using a string literal for the identifier here and in the
method below, it might be better to declare a private variable to avoid
spelling mistakes and the ability to rename this identifier in a single
place.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmyViewController.swift
<#13 (comment)>
:
> + @IBOutlet var textField: UITextField!
+
+ override func viewDidLoad() {
+
+
+ }
+
+
+ @IBAction func buttonPressed(_ sender: Any) {
+ performSegue(withIdentifier: "sendDataForwards", sender: self)
+ }
+
+ override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
+ if segue.identifier == "sendDataForwards" {
+
+ let secondVC = segue.destination as! JimmySecondViewController
A guard > force unwrapping.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmyViewController.swift
<#13 (comment)>
:
> + override func viewDidLoad() {
+
+
+ }
+
+
+ @IBAction func buttonPressed(_ sender: Any) {
+ performSegue(withIdentifier: "sendDataForwards", sender: self)
+ }
+
+ override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
+ if segue.identifier == "sendDataForwards" {
+
+ let secondVC = segue.destination as! JimmySecondViewController
+
+ secondVC.data = textField.text!
The guard above should also check for the text field contents, rather than
force unwrapping.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmyViewController.swift
<#13 (comment)>
:
> + @IBAction func buttonPressed(_ sender: Any) {
+ performSegue(withIdentifier: "sendDataForwards", sender: self)
+ }
+
+ override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
+ if segue.identifier == "sendDataForwards" {
+
+ let secondVC = segue.destination as! JimmySecondViewController
+
+ secondVC.data = textField.text!
+
+ secondVC.delegate = self
+ }
+ }
+
+ func dataRecieved(data: String) {
This is totally fine, though I am a fan of creating a separate extension
that conforms to the delegate protocol and have the delegate methods there
for tidiness.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/MainTableViewController.swift
<#13 (comment)>
:
> default:
assertionFailure("Cell not configured yet")
}
-
+
+ if indexPath.row == 10 {
+ let storyboard = UIStoryboard(name: "Jimmy", bundle: nil)
+ let destination = storyboard.instantiateViewController(withIdentifier: "JimmyViewController") as! JimmyViewController
💂
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/Jimmy.storyboard
<#13 (comment)>
:
> + <color key="titleColor" white="1" alpha="1" colorSpace="custom" customColorSpace="genericGamma22GrayColorSpace"/>
+ </state>
+ <connections>
+ <action selector="buttonPressed:" destination="JQL-P8-rL1" eventType="touchUpInside" id="bwV-ls-74l"/>
+ </connections>
+ </button>
+ <textField opaque="NO" contentMode="scaleToFill" contentHorizontalAlignment="left" contentVerticalAlignment="center" borderStyle="roundedRect" placeholder="type something to beam up!" textAlignment="center" minimumFontSize="17" translatesAutoresizingMaskIntoConstraints="NO" id="Id6-G3-T4c">
+ <rect key="frame" x="20" y="314" width="374" height="40"/>
+ <constraints>
+ <constraint firstAttribute="height" constant="40" id="9zj-IF-OEb"/>
+ </constraints>
+ <nil key="textColor"/>
+ <fontDescription key="fontDescription" type="system" pointSize="25"/>
+ <textInputTraits key="textInputTraits"/>
+ </textField>
+ <label opaque="NO" userInteractionEnabled="NO" contentMode="left" horizontalHuggingPriority="251" verticalHuggingPriority="251" text="Label" textAlignment="natural" lineBreakMode="tailTruncation" baselineAdjustment="alignBaselines" adjustsFontSizeToFit="NO" translatesAutoresizingMaskIntoConstraints="NO" id="ASx-aW-Sso">
You may want to add leading/trailing constraints to avoid the text from
disappearing off screen if the label contents are very long.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmySecondViewController.swift
<#13 (comment)>
:
> +//
+// Created by Jimmy Gutierrez on 5/23/19.
+// Copyright © 2019 SwiftMiami. All rights reserved.
+//
+
+import UIKit
+
+protocol canRecieve {
+ func dataRecieved(data: String)
+}
+
+class JimmySecondViewController: UIViewController {
+
+ var delegate : canRecieve?
+
+ var data = ""
Renaming to initialData may make it a little clearer what the role of
this variable is.
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmySecondViewController.swift
<#13 (comment)>
:
> @@ -0,0 +1,41 @@
+//
+// JimmySecondViewController.swift
+// BackToBasicsUIKit
+//
+// Created by Jimmy Gutierrez on 5/23/19.
+// Copyright © 2019 SwiftMiami. All rights reserved.
+//
+
+import UIKit
+
+protocol canRecieve {
+ func dataRecieved(data: String)
The function signature should also include the sender, so the delegate
knows who called it:
func dataRecieved(viewController: UIViewController, data: String)
------------------------------
In BackToBasicsUIKit/BackToBasicsUIKit/Demo
ViewControllers/Jimmy/JimmySecondViewController.swift
<#13 (comment)>
:
> +
+ var delegate : canRecieve?
+
+ var data = ""
+
+ @IBOutlet var label: UILabel!
+ @IBOutlet var textField: UITextField!
+
+ override func viewDidLoad() {
+ super.viewDidLoad()
+ label.text = data
+
+ }
+
+
+ @IBAction func passItBack(_ sender: Any) {
While this pattern works, it assumes that the previous view controller is
the same as its delegate. A slightly better pattern may be to send the data
back to the delegate and then allow the delegate to control any navigation
(in this case popping the VC).
However, a *much better* approach is to use unwind segues here as they
handle all of this for you, without the need of a delegate protocol or
handling the navigation yourself. This is a great tutorial on this type of
segue: https://matteomanferdini.com/unwind-segue/
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#13?email_source=notifications&email_token=AMEYEXAAI23OBQIQQNSZBCTPX7PG5A5CNFSM4HRFHNW2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2EDWSY#pullrequestreview-243809099>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMEYEXFQUS7CKCEWRTLPGULPX7PG5ANCNFSM4HRFHNWQ>
.
|
No description provided.