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

Week6 essential #7

Open
wants to merge 47 commits into
base: develop/view
Choose a base branch
from
Open

Week6 essential #7

wants to merge 47 commits into from

Conversation

briandr97
Copy link
Contributor

No description provided.

…gmin-Kwon into week4-essential

� Conflicts:
�	app/build.gradle
로그인시 id를 입력했던 것이 email을 입력하는 것으로 수정됨에 따라 네이밍과 strings를 수정
인터넷 연결 권한을 추가하고 http통신이 가능하도록 옵션 추가
로그인, 회원가입 서버통신을 위해 해당 Response, Request DTO를 생성했다.
Retrofit2를 이용해 로그인, 회원가입 서버통신할 함수를 만들었다.
Retrofit2 객체 생성하는 부분을 따로 구분했다.
SignUpActivity에서 회원가입 서버통신하는 로직을 구현하고
strings에 필요한 값들을 추가했다.
SignInActivity에서 로그인 서버통신하는 로직을 구현하고
strings에 필요한 값들을 추가했다.
이제 서버통신으로
로그인을 구분하므로 AuthChecking에서 로그인과 관련된 로직들을 삭제했다.
entity 패키지 안에 auth 패키지를 생성
Copy link

@2zerozu 2zerozu left a comment

Choose a reason for hiding this comment

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

잘 보고 갑니다요

  • 커밋 내역이 환상적이네요..

Comment on lines +19 to +21
var loginEmail: String?
get() = loginPreferences.getString("LOGIN_EMAIL", null)
set(value) = editor.putString("LOGIN_EMAIL", value).apply()
Copy link

Choose a reason for hiding this comment

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

데이터타입이 nullable한 이유가 있을까유?!

Choose a reason for hiding this comment

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

이렇게 처리해도 될 것 같은게 어차피 빈 스트링이나 null이나 저장이 안된값으로 인식하면 되어서 처리 로직은 거의 비슷할 듯. 다만 '?.' 연산자를 항상 써야되냐 말아야되나 차이인데 그정도는 취향차이라고 봐도 될 것 같아용

Comment on lines +78 to +79
val ID_RANGE = IntRange(6, 10)
val PW_RANGE = IntRange(6, 12)
Copy link

Choose a reason for hiding this comment

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

👀

Copy link

@l2hyunwoo l2hyunwoo left a comment

Choose a reason for hiding this comment

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

고생했습니다.


//글라이드
implementation 'com.github.bumptech.glide:glide:4.14.2'
annotationProcessor 'com.github.bumptech.glide:compiler:4.14.2'

Choose a reason for hiding this comment

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

Suggested change
annotationProcessor 'com.github.bumptech.glide:compiler:4.14.2'
kapt 'com.github.bumptech.glide:compiler:4.14.2'

@@ -1,6 +1,7 @@
plugins {
id 'com.android.application'
id 'org.jetbrains.kotlin.android'
id 'org.jetbrains.kotlin.plugin.serialization' version('1.7.10')

Choose a reason for hiding this comment

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

Suggested change
id 'org.jetbrains.kotlin.plugin.serialization' version('1.7.10')
id 'org.jetbrains.kotlin.plugin.serialization' version('1.7.10')
id 'kotlin-kapt'

아래에 kapt 기능 사용해야되니까 이거 추가해야됨

Comment on lines +19 to +21
var loginEmail: String?
get() = loginPreferences.getString("LOGIN_EMAIL", null)
set(value) = editor.putString("LOGIN_EMAIL", value).apply()

Choose a reason for hiding this comment

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

이렇게 처리해도 될 것 같은게 어차피 빈 스트링이나 null이나 저장이 안된값으로 인식하면 되어서 처리 로직은 거의 비슷할 듯. 다만 '?.' 연산자를 항상 써야되냐 말아야되나 차이인데 그정도는 취향차이라고 봐도 될 것 같아용

val totalPages: Int
) {
@Serializable
data class Data(

Choose a reason for hiding this comment

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

이것도 플러그인이 만들어주신 이름 같다만 Data라는 이름은 최대한 피해주는게 좋을 것 같아. 가독성/직관성이 떨어져보여

@@ -20,8 +20,7 @@ class SplashActivity : AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
binding = ActivitySplashBinding.inflate(layoutInflater)
sharedPref = MySharedPreferences()
sharedPref.init(this)
sharedPref = MySharedPreferences(this)

Choose a reason for hiding this comment

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

Application 클래스 만들어서 거기서 한번만 init하고 사용하는걸 추천할게용

Comment on lines +3 to +6
enum class EditTextUiState() {
NOT_FOCUSED,
CORRECT,
INCORRECT;

Choose a reason for hiding this comment

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

Suggested change
enum class EditTextUiState() {
NOT_FOCUSED,
CORRECT,
INCORRECT;
enum class EditTextUiState {
NOT_FOCUSED,
CORRECT,
INCORRECT;

어쨌던 굉장히 좋은 선택인 것 같음

val signInResult: LiveData<ServerConnectResult> get() = _signInResult

fun signIn(email: String, pw: String) {
val loginService = ServicePool.authService // ServiceFactory.retrofit.create<AuthService>()

Choose a reason for hiding this comment

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

이건 뷰모델 전역 변수로 빼도 되지 않을까?

import retrofit2.Response

class SignUpViewModel : ViewModel() {
private val authChecking = AuthChecking()

Choose a reason for hiding this comment

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

약간 이 친구가 UseCase 같은 느낌이라 AuthRepository 정도로 이름 바꿔도 될 것 같은 느낌!

class ReqresAdapter : ListAdapter<ResponseReqresDTO.Data, ReqresAdapter.ReqresViewHolder>(ReqresComparator()) {
class ReqresViewHolder(private val binding: ItemReqresBinding) : RecyclerView.ViewHolder(binding.root) {
fun onBind(data: ResponseReqresDTO.Data) {
Glide.with(binding.imageReqresProfile).load(data.avatar).into(binding.imageReqresProfile)

Choose a reason for hiding this comment

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

Coil 써봅시당

import org.sopt.sample.data.remote.entity.reqres.ResponseReqresDTO
import org.sopt.sample.databinding.ItemReqresBinding

class ReqresAdapter : ListAdapter<ResponseReqresDTO.Data, ReqresAdapter.ReqresViewHolder>(ReqresComparator()) {

Choose a reason for hiding this comment

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

Suggested change
class ReqresAdapter : ListAdapter<ResponseReqresDTO.Data, ReqresAdapter.ReqresViewHolder>(ReqresComparator()) {
class ReqresAdapter : ListAdapter<ResponseReqresDTO.Data, ReqresAdapter.ReqresViewHolder>(ReqresComparator) {

ReqresComparator -> object로 사용해도 될 것 같음

Copy link
Member

@IslandofDream IslandofDream left a comment

Choose a reason for hiding this comment

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

코드보면서 배워갑니다😀😀

@@ -0,0 +1,11 @@
package org.sopt.sample.ui.auth

enum class EditTextUiState() {
Copy link
Member

Choose a reason for hiding this comment

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

Uistate활용 좋네요.. 하나 배워갑니다

app:layout_constraintTop_toTopOf="parent"
app:lottie_autoPlay="true"
app:lottie_loop="true"
app:lottie_rawRes="@raw/loading_animation_paper_airplane" />
Copy link
Member

Choose a reason for hiding this comment

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

@@ -25,6 +25,10 @@ fun Context.shortToast(stringId: Int) {
Toast.makeText(this, stringId, Toast.LENGTH_SHORT).show()
}

fun Context.shortToastString(text: String) {
Copy link
Member

@IslandofDream IslandofDream Dec 4, 2022

Choose a reason for hiding this comment

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

생각해보니까 토스트메세지 확장함수는 Context.~~ 로 하는게 적절하겠네요 제 코드는 View.~~ 으로 되어있었는데 아주 좋았습니다!

Copy link

@unam98 unam98 left a comment

Choose a reason for hiding this comment

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

많이 배워갑니당

Comment on lines +3 to +6
enum class ServerConnectResult(val message: String) {
SUCCESS("서버통신이 성공했습니다."),
ON_RESPONSE_FAIL("서버통신이 애매하게 이상합니다."),
ON_FAILURE("서버통신이 전혀 안됐습니다.");
Copy link

Choose a reason for hiding this comment

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

그동안 형 코드들 보면 enum class는 꼭 활용하는 것 같아. 전반적으로 가독성에 신경을 많이 쓰면서 코드를 짜는 것처럼 보여. 나도 다음에 써봐야겠다 굿굿

import android.content.Context
import org.sopt.sample.R
import org.sopt.sample.shortToast

class AuthChecking {
Copy link

Choose a reason for hiding this comment

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

클래스로 따로 빼서 관리해주는 거 좋네요

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

Successfully merging this pull request may close these issues.

5 participants