Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cr.swift file #1

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions cr.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
//
// Netguru iOS code review task
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave information about the author of the file :)

//

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot about importing UIKit :)

class paymentViewController: UIViewController {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I add a keyword 'final' to the view controllers, to avoid inheritance by a mistake.
I consider it a good practice :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

var delegate: PaymentViewControllerDelegate
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add 'weak' keyword to avoid ARC isssues - strong reference cycle

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are sure that this delegate would be never a nil, you can consider using 'unowned' keyword

let customView = PaymentView()
let payment: Payment?
Comment on lines +7 to +8
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider encapsulating those variables, if they won't be used outside the file
public -> private


func callbacks() {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make the function private as it won't be used outside the view controller.
Also, I think that you have named it "setupCallbacks()" somewhere else in the code.

view.backgroundColor = UIColor(displayP3Red: 1, green: 1, blue: 1, alpha: 1)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

view.backgroundColor = .white
would look even more neat 😄

You can also use XCAssets as we might implement dark mode in the future.
I'll be changing it in a while, I'm implementing SwiftGen right now 😉

customView.didTapButton = {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

customView.didTapButton = { [weak self] in to avoid ARC

if let payment = self.payment {
self.delegate.didFinishFlow(amount: payment.amount
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you forgot about a comma :)

currency: payment.currency)
}
}
}

func viewDidLoad() {
navigationController?.setNavigationBarHidden(false, animated: false)
customView.centerXAnchor.constraint(equalTo: view.centerXAnchor)
customView.centerYAnchor.constraint(equalTo: view.centerYAnchor)
Comment on lines +22 to +23
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no isActive = true for those constraints

view.addSubview(customView)
fetchPayment()
setupCallbacks()
}

internal let ViewModel: PaymentViewModel
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable names should be in camel case

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would also bring it to the top, along the other variables


func fetchPayment() {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let it have some privacy 😄
I would move the whole method to the PaymentViewModel :)

customView.statusText = "Fetching data"
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this string to our L18n struct, it is a good practice to not leave hardcoded variables in controllers :)

ApiClient.sharedInstance().fetchPayment { payment in
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ [weak self] payment in :)

self.CustomView.isEuro = payment.currency == "EUR" ? true : false
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compare it by enum, not it's value - in some cases, it may change in a future

if payment!.amount != 0 {
Copy link
Owner Author

@adamgolczak adamgolczak Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guard would be a far better solution in my opinion
force unwrap is a bad practice

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, I would consider a isZero, but it's not a necessity :)

self.CustomView.label.text = "\(payment!.amount)"
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, you have misspelled customView name :)

return
}
}
}
}