CR file Change #1
Conversation
zeeshansuleman
left a comment
zeeshansuleman
left a comment
There was a problem hiding this comment.
Couple of comments added to improve the code.
|
|
||
| func callbacks() { | ||
| view.backgroundColor = UIColor(displayP3Red: 1, green: 1, blue: 1, alpha: 1) | ||
| customView.didTapButton = { |
There was a problem hiding this comment.
Closure must be used with capture list to avoid memory leaks.
| let payment: Payment? | ||
|
|
||
| func callbacks() { | ||
| view.backgroundColor = UIColor(displayP3Red: 1, green: 1, blue: 1, alpha: 1) |
There was a problem hiding this comment.
create a color extension and define proper name for the colors to increase code readability, Also it will be useful to reuse the color at different places.
|
|
||
| class paymentViewController: UIViewController { | ||
| var delegate: PaymentViewControllerDelegate | ||
| let customView = PaymentView() |
There was a problem hiding this comment.
should be "lazy" var to load in memory when used.
| // | ||
|
|
||
| class paymentViewController: UIViewController { | ||
| var delegate: PaymentViewControllerDelegate |
There was a problem hiding this comment.
Use "weak" for the delegates to avoid retain cycle.
| } | ||
|
|
||
| func viewDidLoad() { | ||
| navigationController?.setNavigationBarHidden(false, animated: false) |
There was a problem hiding this comment.
super call for viewdidload is missing.
There was a problem hiding this comment.
Also please move all the code from view did load method to a seperate method e.g setupInterface()
| setupCallbacks() | ||
| } | ||
|
|
||
| internal let ViewModel: PaymentViewModel |
There was a problem hiding this comment.
first character of variable name must be lowercase.
|
|
||
| func fetchPayment() { | ||
| customView.statusText = "Fetching data" | ||
| ApiClient.sharedInstance().fetchPayment { payment in |
There was a problem hiding this comment.
use unowned capture list in closure.
| customView.statusText = "Fetching data" | ||
| ApiClient.sharedInstance().fetchPayment { payment in | ||
| self.CustomView.isEuro = payment.currency == "EUR" ? true : false | ||
| if payment!.amount != 0 { |
There was a problem hiding this comment.
avoid force unwrapping and use if let or guard statement to avoid crash if the value is nil
| ApiClient.sharedInstance().fetchPayment { payment in | ||
| self.CustomView.isEuro = payment.currency == "EUR" ? true : false | ||
| if payment!.amount != 0 { | ||
| self.CustomView.label.text = "\(payment!.amount)" |
There was a problem hiding this comment.
remove force unwrapping here as well.
No description provided.