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] 4차 세미나 프로젝트 추가 (#7) #8

Closed
wants to merge 11 commits into from

Conversation

jumining
Copy link
Member

@jumining jumining commented Nov 14, 2021

📌 관련 이슈

closed #7

📌 변경 사항 및 이유

서버 통신 구현 과제 입니다.

📌 PR Point

UserSignService.swift에서 signUp 함수와 login 함수 내용이 url빼고 겹치는 것 같긴한데 줄일 수 있을까용..?
줄이는 게 더 나은게 맞을까요??

📌 참고 사항

다른 지적 사항 있으면 알려주세용!

Copy link
Member

@heerucan heerucan left a comment

Choose a reason for hiding this comment

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

전반적으로 스쉐 코드 컨벤션 보고 참고해서 잘 맞춰주면 좀 더 깔끔하고 좋을 것 같아요.
띄어쓰기, 공백 같은 사소한 것만 맞춰주면 통일감 잇어서 되게 좋을 것 같고,,
음,, 추가로 constant, extension 만들어서 폰트, identifier, color, asset 관리하는 연습도 해보면 좋겠어용
회원가입 그때 물어봤었는데 잘해왔네여!!!!

Comment on lines +20 to +22
// MARK: - Life Cycle Part
override func viewDidLoad() {
super.viewDidLoad()
Copy link
Member

Choose a reason for hiding this comment

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

여기 컨벤션 지켜서 빈 줄 하나 추가해주세요.

Comment on lines +17 to +18
self.tabBar.tintColor = .black
self.tabBar.unselectedItemTintColor = .gray
Copy link
Member

Choose a reason for hiding this comment

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

생명주기에 함수만 넣어주쇼랑


@IBAction func touchUpOkayButton(_ sender: Any) {
guard let nextVC = self.storyboard?.instantiateViewController(withIdentifier: "CustomTabBarController") as? CustomTabBarController
else {return}
Copy link
Member

Choose a reason for hiding this comment

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

{ return } 이라고 컨벤션 맞춰주면 좋겠습니다.. 컨벤션무새가 되어버린 나..

Comment on lines +31 to +45
homeVC.tabBarItem.title="홈"
homeVC.tabBarItem.image=UIImage(named: "homeIconFill")
homeVC.tabBarItem.selectedImage = UIImage(named: "homeIcon")
shortsVC.tabBarItem.title="shorts"
shortsVC.tabBarItem.image=UIImage(named: "shortsIcon")
shortsVC.tabBarItem.selectedImage=UIImage(named: "shortsIconFill")
plusVC.tabBarItem.title="추가"
plusVC.tabBarItem.image=UIImage(named: "plueCircleIcon")
plusVC.tabBarItem.selectedImage=UIImage(named: "plueCircleIcon")
subscriptionVC.tabBarItem.title="구독"
subscriptionVC.tabBarItem.image=UIImage(named: "subscriptionsIcon")
subscriptionVC.tabBarItem.selectedImage=UIImage(named: "subscriptionIconsFill")
libraryVC.tabBarItem.title="저장"
libraryVC.tabBarItem.image=UIImage(named: "LibraryIcon")
libraryVC.tabBarItem.selectedImage=UIImage(named: "LibraryIconFill")
Copy link
Member

Choose a reason for hiding this comment

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

= 여기 양 사이드로도 공백 하나씩 해서 맞춰주세요~!

Comment on lines +67 to +76
if message == "로그인 성공" {
let okAction = UIAlertAction(title: "확인", style: .default) {(action) in
self.goToCheckView() }
alert.addAction(okAction)
present(alert, animated: true)
} else {
let okAction = UIAlertAction(title: "확인", style: .default)
alert.addAction(okAction)
present(alert, animated: true)
}
Copy link
Member

Choose a reason for hiding this comment

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

성공할 때만 self.goToCheckView() 해준 거 잘했는데 그 외의 중복되는 건 따로 빼주면 좋을 거 가텅

Comment on lines +30 to +33
videoTableView.dataSource = self
videoTableView.delegate = self
youtuberCollectionView.delegate = self
youtuberCollectionView.dataSource = self
Copy link
Member

Choose a reason for hiding this comment

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

얘도 따로 빼주셔라..

}
}

// MARK: - Extension Part
Copy link
Member

Choose a reason for hiding this comment

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

보통 extension을 하나로 묶지 않고 마크구문을
위에 따로 하나하나 다 써줘야 합니당
UITableViewDelegate
UITableViewDataSource
UICollectionViewDataSource
UICollectionViewDelegateFlowLayout


// MARK: - LoginResponseData

struct LoginResponseData: Codable {
Copy link
Member

Choose a reason for hiding this comment

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

하나의 모델을 공유해서 쓸 거면.. AuthResponseData 같이 통합해서 써주면 좋을 것 같아융

Comment on lines +27 to +31
let body: Parameters = [
"email": email,
"name" : name,
"password": password
]
Copy link
Member

Choose a reason for hiding this comment

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

로그인 서버 위키 보면 요청 바디에 name이 없을 겁니다 다시 확인해보세요!

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 잘 확인안하고 써버렸네요..감사합니당!

Comment on lines +23 to +24
let header: HTTPHeaders = [
"Content-Type" : "application/json"
Copy link
Member

Choose a reason for hiding this comment

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

헤더는 같으니까 함수 밖에 빼주면 같이 로그인, 회원가입 함수 안에 같이 써줄 수 있을 것 같아요!
그리고 url 같은 경우도 함수 밖에 빼서
let url = APIConstants.baseURL 이라고 쓰면
로그인은 url.loginURL
회원가입은 url.signUpURL 이라고 쓸 수 잇을 거 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

이부분도 따로 빼서 수정해보려 했는데 잘 모르겠어요..ㅜ 개인적으로 나중에 물어보겠습니다ㅎ

Comment on lines +50 to +62
func simpleAlert(message: String) {
let alert = UIAlertController(title: "회원가입", message: message, preferredStyle: .alert)
if message == "회원 가입 성공" {
let okAction = UIAlertAction(title: "확인", style: .default) {(action) in
self.goToCheckView() }
alert.addAction(okAction)
present(alert, animated: true)
} else {
let okAction = UIAlertAction(title: "확인", style: .default)
alert.addAction(okAction)
present(alert, animated: true)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

우리가 보통 함수로 따로 분리를 한다는 뜻은 -
"특정 행동을 수행하도록 도와주는 코드의 모음" 이라고 볼 수 있을 것 같어!!

그래서 보통 우리가 함수를 작성하고 - 이름을 작성하는 이유가
"이 함수는 이런 동작을 하는 친구들이에요"
라는걸 알리기 위해서거든?!

그래서 보통 하나의 함수를 작성할때에는
"하나의 역할"만 하도록 함수를 작성하는게 일반적인 이야기일거야!!

위 이야기를 한 이유는 -
지금 simpleAlert 라는 함수에서는 UIAlertController를 present하는 함수 형태로 작성한건데
self.goToCheckView() 라구 화면 이동 하는 기능이 같이 포함되어 있는 형태라서

여기서 조금 더 분리된 형태로 함수를 제작 할 수 있을 것 같어!!
simpleAlert를 띄우고, 파라미터로, OK를 눌렀을 때 동작하는 것을 클로저 형태로 받아오는 식으로 함수를 작성 할 수 있겠지?

앞으로 우리가 코드를 작성할 때,
최대한 1함수 1기능을 하도록 생각을 해보자!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

1함수 1기능 명심하겠습니다!
수정해볼테니 다음 세미나 과제에서 이 코드 부분을 쓴다면 코드리뷰에서 잘 쓴 게 맞는지 확인 부탁드립니당..,,!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] iOS 4차 과제
3 participants