This repository was archived by the owner on Mar 5, 2025. It is now read-only.
Develop #1
Open
Develop #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/API.swift
Outdated
| return json.arrayValue.map{ | ||
| Model.Issue(json: $0) | ||
| } | ||
| }) |
There was a problem hiding this comment.
뭔가 좀더 간단히 할 수 있을 것 같은데..
let result = dataResponse.map { $0.arrayValue }.map { Model.Issue(json: $0) }
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/API.swift
Outdated
| } | ||
|
|
||
| typealias IssueResponsesHandler = (DataResponse<[Model.Issue]>) -> Void | ||
| static func repoIssues(owner: String, repo: String) -> (Int, @escaping IssueResponsesHandler) -> Void { |
There was a problem hiding this comment.
typealias는 class 밖에 있는게 더 읽기 편할듯?
There was a problem hiding this comment.
API. IssueResponsesHandler 보다 IssueResponsesHandler가 나아보이나요?
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/AppDelegate.swift
Outdated
| if !GlobalState.instance.isLoggedIn { | ||
| let loginViewController = LoginViewController.viewController | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.0, execute: { [weak self] in | ||
| self?.window?.rootViewController?.present(loginViewController, animated: false, completion: nil) |
There was a problem hiding this comment.
여기야 말로 unowned로 써도 되겠네요?
아니면... 이렇게 회피? ㅋㅋㅋ
UIApplication.shared.delegate?.window?.rootViewController?.present(loginViewController, animated: false, completion: nil)
yoonhg84
reviewed
Sep 26, 2017
| print(issue) | ||
| self.createdIssue = issue | ||
| self.performSegue(withIdentifier: "UnwindToIssues", sender: self) | ||
| case .failure(_): |
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/GlobalState.swift
Outdated
| static let tokenKey = "token" | ||
| static let ownerKey = "owner" | ||
| static let repoKey = "repo" | ||
| static let reposKey = "repos" |
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/GlobalState.swift
Outdated
| var token: String? { | ||
| get { | ||
| let token = UserDefaults.standard.string(forKey: constants.tokenKey) | ||
| return token |
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/Issue.swift
Outdated
| "body": body, | ||
| "state": state.display, | ||
| "user": ["id": user.id, "login": user.login, "acatar_url": (user.avatarURL?.absoluteString ?? "")] | ||
| ] as [String : Any] |
There was a problem hiding this comment.
그리고 결국 타입이 [String:Any] 라면 변수에 적어주는게 더 좋겠네요.
var dict: [String : Any] = [
"id": id,
"number": number,
"title": title,
"comments": comments,
"body": body,
"state": state.display,
"user": [
"id": user.id,
"login": user.login,
"acatar_url": (user.avatarURL?.absoluteString ?? "")
]
]
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/Issue.swift
Outdated
| enum State: String { | ||
| case open = "open" | ||
| case closed = "closed" | ||
| case none = "none" |
There was a problem hiding this comment.
String 을 상속받고 case 와 rawValue 가 같으면 rawValue 따로 안 써도 됩니다.
enum State: String {
case open
case closed
case none
}
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/Issue.swift
Outdated
| switch self { | ||
| case .open: return "opened" | ||
| case .closed: return "closed" | ||
| default: return "-" |
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/IssueCell.swift
Outdated
|
|
||
| static var cellFromNib: IssueCell { | ||
| get { | ||
| return Bundle.main.loadNibNamed("IssueCell", owner: nil, options: nil)?.first as! IssueCell |
There was a problem hiding this comment.
오류날 일은 없겠지만... 그래도 as!는 위험하니
guard let cell = Bundle.main.loadNibNamed("IssueCell", owner: nil, options: nil)?.first as? IssueCell else {
assertionFailure()
return IssueCell()
}
return cell
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/IssueCell.swift
Outdated
| } | ||
| } | ||
|
|
||
| func update(data issue: Model.Issue) { |
There was a problem hiding this comment.
func update(issue: Model.Issue) {
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/IssueCommentCell.swift
Outdated
| final class IssueCommentCell: UICollectionViewCell, CellProtocol { | ||
|
|
||
|
|
||
| @IBOutlet var bodyLabel: UILabel! |
yoonhg84
reviewed
Sep 26, 2017
|
|
||
| @IBOutlet var commentTextField: UITextField! | ||
| var owner: String = "" | ||
| var repo: String = "" |
There was a problem hiding this comment.
빈줄이 IBOutlet 두개 사이가 아니라 owner 와 commentTextField 사이에 들어가는게 더 보기 좋을 것 같네요
yoonhg84
reviewed
Sep 26, 2017
| var owner: String = "" | ||
| var repo: String = "" | ||
| var issue: Model.Issue! | ||
| var headerSize: CGSize = CGSize.zero |
There was a problem hiding this comment.
줄임 가능
var headerSize: CGSize = .zero
yoonhg84
reviewed
Sep 26, 2017
| @IBAction func sendButtonTapped(_ sender: Any) { | ||
| let comment = commentTextField.text ?? "" | ||
| API.createComment(owner: owner, repo: repo, number: issue.number, comment: comment) { [weak self] (dataResponse: DataResponse<Model.Comment>) in | ||
| guard let `self` = self else { return } |
There was a problem hiding this comment.
이거에 대한 의견도 많죠.
전 self는 안 쓰자는 주의 ㅋ
[weak self] 를 놓쳤을때 찾기가 쉽지 않아서..
yoonhg84
reviewed
Sep 26, 2017
|
|
||
| default: | ||
|
|
||
| assert(false, "Unexpected element kind") |
There was a problem hiding this comment.
assertionFailure("Unexpected element kind")
yoonhg84
reviewed
Sep 26, 2017
|
|
||
| } | ||
| override func prepare(for segue: UIStoryboardSegue, sender: Any?) { | ||
| if let detailViewController = segue.destination as? IssueDetailViewController,let issue = sender as? Model.Issue { |
There was a problem hiding this comment.
여기서는 segue.identifier 인가로 비교하는게 좋지 않나요?
There was a problem hiding this comment.
분기의 목적보다는 까려는 목적? 어케 생각하시나요?
yoonhg84
reviewed
Sep 26, 2017
|
|
||
| case UICollectionElementKindSectionHeader: | ||
| assert(false, "Unexpected element kind") | ||
|
|
There was a problem hiding this comment.
이렇게 사용하시면 release 빌드에서 오류납니다.
assert외에 다른 문장이 없으면 항상 break를 같이 넣어주세요
yoonhg84
reviewed
Sep 26, 2017
| estimatedLayout[key] = nil | ||
| } else { | ||
| estimatedLayout = [:] | ||
| } |
There was a problem hiding this comment.
개인 취향
guard let key = indexPath else {
estimatedLayout = [:]
return
}
estimatedLayout[key] = nil
yoonhg84
reviewed
Sep 26, 2017
| func estimateLayoutAttributes(_ layoutAttributes: UICollectionViewLayoutAttributes) -> UICollectionViewLayoutAttributes { | ||
| if layoutAttributes.isHidden { | ||
| return layoutAttributes | ||
| } |
There was a problem hiding this comment.
guard layoutAttributes.isHidden == false else { return layoutAttributes }
yoonhg84
reviewed
Sep 26, 2017
| if needRefreshDatasource { | ||
| datasource = [] | ||
| needRefreshDatasource = false | ||
| } |
There was a problem hiding this comment.
이거야 말로 guard를 써야 할때
guard needRefreshDatasource == true else { return }
datasource = []
needRefreshDatasource = false
yoonhg84
reviewed
Sep 26, 2017
| var page: Int = 1 | ||
| var canLoadMore: Bool = true | ||
| fileprivate var estimatedSizes: [IndexPath: CGSize] = [:] | ||
| fileprivate let estimateCell: CellType = CellType.cellFromNib |
There was a problem hiding this comment.
멤버변수 선언이 정신이 없네요.
accessor 별로 분류 하거나 기능별로 분류하거나 뭔가 기준이 있으면 좋을 것 같아요
yoonhg84
reviewed
Sep 26, 2017
| var api: ((Int, @escaping IssueResponsesHandler) -> Void)? | ||
|
|
||
| func loadMore(indexPath: IndexPath) { | ||
| guard indexPath.item == datasource.count - 1 && !isLoading && canLoadMore else { return } |
yoonhg84
reviewed
Sep 26, 2017
| } | ||
|
|
||
| func load() { | ||
| guard isLoading == false else {return } |
yoonhg84
reviewed
Sep 26, 2017
| func load() { | ||
| guard isLoading == false else {return } | ||
| isLoading = true | ||
| api?(page, {[weak self] (response: DataResponse<[Item]>) -> Void in |
There was a problem hiding this comment.
api?(page) { [weak self] (response: DataResponse<[Item]>) -> Void in
yoonhg84
reviewed
Sep 26, 2017
| canLoadMore = false | ||
| loadMoreCell?.loadDone() | ||
|
|
||
| } |
yoonhg84
reviewed
Sep 26, 2017
| 가져온 사이즈를 인덱싱. | ||
| 그 사이즈를 리턴. | ||
|
|
||
| */ |
yoonhg84
reviewed
Sep 26, 2017
|
|
||
|
|
||
| @IBAction func unwindFromRepos(_ segue: UIStoryboardSegue) { | ||
| if let reposViewController = segue.source as? ReposViewController, let (owner, repo) = reposViewController.selectedRepo { |
yoonhg84
reviewed
Sep 26, 2017
| GlobalState.instance.token = "" | ||
| let loginViewController = LoginViewController.viewController | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.0, execute: { [weak self] in | ||
| self?.present(loginViewController, animated: true, completion: nil) |
There was a problem hiding this comment.
DispatchQueue.main.asyncAfter(deadline: .now() + 0.0) { [weak self] in
yoonhg84
reviewed
Sep 26, 2017
GithubIssues/Router.swift
Outdated
| return .put | ||
| case .repoIssues, | ||
| .issueDetail | ||
| : |
There was a problem hiding this comment.
API는 한개씩 추가되니까 아래줄에 하나씩만 추가하려고요
yoonhg84
reviewed
Sep 26, 2017
| let rect = CGRect(x: 0, y: 0, width: size.width, height: size.height) | ||
| UIGraphicsBeginImageContext(rect.size) | ||
|
|
||
| if let context = UIGraphicsGetCurrentContext() { |
intmain
commented
Sep 26, 2017
GithubIssues/API.swift
Outdated
| } | ||
|
|
||
| typealias IssueResponsesHandler = (DataResponse<[Model.Issue]>) -> Void | ||
| static func repoIssues(owner: String, repo: String) -> (Int, @escaping IssueResponsesHandler) -> Void { |
There was a problem hiding this comment.
API. IssueResponsesHandler 보다 IssueResponsesHandler가 나아보이나요?
GithubIssues/IssueCell.swift
Outdated
| } | ||
| } | ||
|
|
||
| func update(data issue: Model.Issue) { |
|
|
||
| } | ||
| override func prepare(for segue: UIStoryboardSegue, sender: Any?) { | ||
| if let detailViewController = segue.destination as? IssueDetailViewController,let issue = sender as? Model.Issue { |
There was a problem hiding this comment.
분기의 목적보다는 까려는 목적? 어케 생각하시나요?
GithubIssues/Router.swift
Outdated
| return .put | ||
| case .repoIssues, | ||
| .issueDetail | ||
| : |
There was a problem hiding this comment.
API는 한개씩 추가되니까 아래줄에 하나씩만 추가하려고요
added 2 commits
October 1, 2017 22:11
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
reqeust