-
Notifications
You must be signed in to change notification settings - Fork 4
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
Assignment - Fix Calculator and make NetworkModel #12
base: changgyo
Are you sure you want to change the base?
Conversation
I make Network Module It's looks like Alamofire description write on the code and fix Calculator
@@ -14,7 +14,7 @@ struct CalculatorApp: App { | |||
|
|||
var body: some Scene { | |||
WindowGroup { | |||
ContentView(calculatorManager: caculatorManager) | |||
ContentView().environmentObject(caculatorManager) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caculatorManager -> ca"l"culatorManager
print("error") | ||
var showingText: String | ||
private(set) var previousNumber: Decimal? { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 왜 공백을 다 주도록 선택했나요 ? 오히려 보기 힘든 거 같은데
@@ -8,27 +8,40 @@ | |||
import SwiftUI | |||
|
|||
struct ContentView: View { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네이밍
@@ -14,7 +14,7 @@ struct CalculatorApp: App { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
계산기는 공백이나, 오타 등이 수정이 안돼서 천천히 읽어보면서 확인해 주세요.
리뷰는 보류합니다.
함수의 시작부에 공백은 하지말고, 구조체나 클래스, 확장 때에만 쓸지 말지 정해주세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다시 수정하겠습니다
|
||
struct NetworkModule { | ||
|
||
static let shared = NetworkModule() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shared를 만든 이유는 무엇인가요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이걸 부르는 모든 곳에서 같은 행동을 할 것이고, 따라서 부를때마다 인스턴스를 만드는 것보다 정적으로 만드는 것이 좋다고 생각했습니다.
또 url session에서 shared 를 사용해서 하기때문에 굳이 인스턴스를 각자 만들어도 session에서 staic하게 동작하기 때문에 static으로 하지 않는 것은 손해라고 생각했습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
싱글턴 패턴의 효과를 보려는 건가요 ?
근데 NetworkModule은 인스턴스를 만들어 낼 수 있지 않나요 ?
싱글턴 패턴의 단점은 무엇일까요 ?
/// - paramaters: 원하는 파라미터를 넣으면 됩니다. 이때. value는 반드시 string protocol을 따라야합니다. | ||
/// - header: 토큰이나 필수 요소를 파라미터와 같은 형태로 넣으면 됩니다. | ||
/// - completion: 제네릭의 넣은 모델을 바탕으로 결과값을 보내줍니다. | ||
func fetchData<EntityType: Decodable>(_ url: String, entity: EntityType.Type, httpMethod: HttpMethod, paramaters: [String: String]? = nil, header: [String: String]? = nil,completion: @escaping (NetworkResult<EntityType>?) -> Void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매개변수가 많습니다. 줄일 수 있지 않을까요 ?
사용자 정의 타입, struct를 활용하는 건 어떤가요 ?
요청이 실패한 것도 알려야 하지 않을까요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정하겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
질문이 있습니다. 만약에 struct로 parameter들을 한 번 정리해서 하는 것도 해당 구조체를 초기화 할 때, 똑같은 과정이 필요한고, 그걸 사용하는 함수내에서도 struct.url 이렇게 써야 할 거 같아서 안했는데 struct로 파라미터를 하는게 더 좋을까요?
요청 실패는 처리하겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 요청에 대한 설정을 하는 것을 메소드에 호출에서 맡기는 것보다,
요청을 설정하는 부분을 외부로 빼고 메소드는 요청 작업만 신경쓰도록 하는 것은 어떤가요 ?
guard let reponseData = try? decoder.decode(EntityType.self, from: data) | ||
else { return } | ||
|
||
switch httpURLResponse.statusCode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NaverOpenAPI의 Status Code를 제대로 읽어봤나요 ?
struct ContentView: View { | ||
|
||
@ObservedObject var viewModel: Finder | ||
@State var movieTitle: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@State 는 해당 뷰의 내부에서만 사용되므로 private으로 막아줘야 합니다.
paramaters?.forEach { key, value in | ||
queryItems.append(URLQueryItem(name: key, value: value)) | ||
} | ||
urlComponent?.queryItems = queryItems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http method 가 POST 인 경우에도 위 동작이 필요한가요~?
} | ||
urlComponent?.queryItems = queryItems | ||
|
||
guard let url = urlComponent?.url else { return } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
열심히 만들고 있는 모듈이니 에러 처리 등을 좀 더 신경써볼까요?
|
||
completion(networkResult) | ||
}).resume() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetchData method 가 담고 있는 역할과 책임이 너무 많은 것 같아요
만들고 처리하는 부분을 조금씩 나눠보면 어떨까요?
static let baseURL = "https://openapi.naver.com/v1" | ||
static let searchURL = baseURL + "/search" | ||
static let movieJsonURL = searchURL + "/movie.json" | ||
static let ClientID = "SjK7VjRrkujvwETKupas" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C c
} | ||
} | ||
|
||
protocol RequestEnum { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protocol을 쓴 이유는 무엇일까요?
class Finder: ObservableObject { | ||
|
||
@Published var movieList: [Movie]? | ||
@Published var testResult: UIImage = UIImage(named: "tempImage")! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!!!
|
||
case .success(let data): | ||
DispatchQueue.main.async { [weak self] in | ||
print(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print(data) | ||
guard let self = self else { return } | ||
self.movieList = data.items | ||
guard let data = try? Data(contentsOf: URL(string: data.items.first?.image ?? "")!), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가급적 강제 언래핑을 지양하도록 해요~
네트워크모듈과 파라미터에 대한 설명은 저번 시간에 알려주신 주석의 방법으로 적어놓았습니다.
모듈을 바탕으로 뷰에서 이미지만 볼 수 있도록 간단한 테스트 뷰를 만들었습니다.