Add cr.swift file #1
Conversation
| // | ||
|
|
||
| class paymentViewController: UIViewController { | ||
| var delegate: PaymentViewControllerDelegate |
There was a problem hiding this comment.
You should add 'weak' keyword to avoid ARC isssues - strong reference cycle
There was a problem hiding this comment.
If you are sure that this delegate would be never a nil, you can consider using 'unowned' keyword
| let customView = PaymentView() | ||
| let payment: Payment? |
There was a problem hiding this comment.
Consider encapsulating those variables, if they won't be used outside the file
public -> private
| // Netguru iOS code review task | ||
| // | ||
|
|
||
| class paymentViewController: UIViewController { |
There was a problem hiding this comment.
I add a keyword 'final' to the view controllers, to avoid inheritance by a mistake.
I consider it a good practice :)
There was a problem hiding this comment.
I would also add a 'PaymentViewControllerProtocol' to satisfy dependency inversion principle which says: "let’s make the high level and the low level structures depend on abstractions".
| // | ||
| // Netguru iOS code review task | ||
| // | ||
|
|
There was a problem hiding this comment.
You forgot about importing UIKit :)
| @@ -0,0 +1,41 @@ | |||
| // | |||
| // Netguru iOS code review task | |||
There was a problem hiding this comment.
I would leave information about the author of the file :)
| 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.
guard would be a far better solution in my opinion
force unwrap is a bad practice
There was a problem hiding this comment.
also, I would consider a isZero, but it's not a necessity :)
| 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.
oh, you have misspelled customView name :)
| customView.centerXAnchor.constraint(equalTo: view.centerXAnchor) | ||
| customView.centerYAnchor.constraint(equalTo: view.centerYAnchor) |
There was a problem hiding this comment.
There is no isActive = true for those constraints
| setupCallbacks() | ||
| } | ||
|
|
||
| internal let ViewModel: PaymentViewModel |
There was a problem hiding this comment.
variable names should be in camel case
There was a problem hiding this comment.
i would also bring it to the top, along the other variables
| view.backgroundColor = UIColor(displayP3Red: 1, green: 1, blue: 1, alpha: 1) | ||
| customView.didTapButton = { | ||
| if let payment = self.payment { | ||
| self.delegate.didFinishFlow(amount: payment.amount |
There was a problem hiding this comment.
you forgot about a comma :)
No description provided.