Conversation
l2hyunwoo
approved these changes
Dec 16, 2021
Comment on lines
+28
to
+44
| <activity | ||
| android:name=".view.profile.RepositoryFragment" | ||
| android:exported="true" | ||
| tools:ignore="Instantiatable" /> | ||
| <activity | ||
| android:name=".view.profile.FollowerFragment" | ||
| android:exported="true" | ||
| tools:ignore="Instantiatable" /> | ||
| <activity | ||
| android:name=".view.login.SignUpActivity" | ||
| android:exported="true" /> | ||
| <activity | ||
| android:name=".view.home.HomeActivity" | ||
| android:exported="true" /> | ||
| <activity | ||
| android:name=".view.login.SignInActivity" | ||
| android:exported="true"></activity> |
There was a problem hiding this comment.
exported 속성이 어떤 것인지 확인해보면 좋을 것 같습니다.
Comment on lines
+9
to
+19
| fun getAutoLogin(context: Context) : Boolean{ | ||
| val preferences = context.getSharedPreferences(USER_AUTH, Context.MODE_PRIVATE) | ||
| return preferences.getBoolean(AUTO_LOGIN, false) | ||
| } | ||
|
|
||
| fun setAutoLogin(context: Context, auto : Boolean) { | ||
| val preferences = context.getSharedPreferences(USER_AUTH, Context.MODE_PRIVATE) | ||
| preferences.edit() | ||
| .putBoolean(AUTO_LOGIN,auto) | ||
| .apply() | ||
| } |
There was a problem hiding this comment.
val preferences = context.getSharedPreferences(USER_AUTH, Context.MODE_PRIVATE)가 SOPTSharedPreferences에서 계속 사용되고 있는데 멤버변수로 한번 초기화 시키고 사용하는게 중복을 덜 수 있지 않을까요?
Comment on lines
+21
to
+26
|
|
||
| //initTransactionEvent() | ||
| initAdapter() | ||
| initBottomNavigation() | ||
|
|
||
| setContentView(binding.root) |
Comment on lines
+30
to
+43
| // fun initTransactionEvent() { | ||
| // val followerFragment = FollowerFragment() | ||
| // val repositstoryFragment = RepositoryFragment() | ||
| // | ||
| // supportFragmentManager.beginTransaction().add(R.id.container_rv, followerFragment).commit() | ||
| // | ||
| // binding.btnFollower.setOnClickListener { | ||
| // supportFragmentManager.beginTransaction().replace(R.id.container_rv, followerFragment) .commit() | ||
| // } | ||
| // | ||
| // binding.btnRepository.setOnClickListener { | ||
| // supportFragmentManager.beginTransaction().replace(R.id.container_rv, repositstoryFragment) .commit() | ||
| // } | ||
| // } |
| private fun initAdapter() { | ||
| val fragmentList = listOf(ProfileFragment(), HomeFragment(), CameraFragment()) | ||
|
|
||
| SampleViewPagerAdapter = SampleViewPagerAdapter(this) |
There was a problem hiding this comment.
- 변수명은 camelCase가 원칙입니다
- 변수명과 클래스명이 동일하면 다른 협업하는 사람의 입장에서 혼란을 야기할 수 있을 것 같습니다.
| import android.view.ViewGroup | ||
| import com.example.myapplication.R | ||
|
|
||
| class home_following_Fragment : Fragment() { |
Comment on lines
+15
to
+17
| private lateinit var OnboardingFragment3: FragmentOnboarding3Binding | ||
| private var _binding: FragmentOnboarding3Binding? = null | ||
| private val binding get() = _binding!! |
There was a problem hiding this comment.
Suggested change
| private lateinit var OnboardingFragment3: FragmentOnboarding3Binding | |
| private var _binding: FragmentOnboarding3Binding? = null | |
| private val binding get() = _binding!! | |
| private var _binding: FragmentOnboarding3Binding? = null | |
| private val binding get() = _binding!! |
이거 지우셔도 되겠죠?
| inflater: LayoutInflater, container: ViewGroup?, | ||
| savedInstanceState: Bundle? | ||
| ): View? { | ||
| _binding = FragmentOnboarding3Binding.inflate(layoutInflater, container, false) |
There was a problem hiding this comment.
_binding 객체를 inflate 했으면 onDestroyView에서 null로 해제를 시켜야 memeory leak이 발생할 가능성이 줄어듭니다.
Comment on lines
+39
to
+42
| override fun onDestroy() { | ||
| super.onDestroy() | ||
| _binding = null | ||
| } |
There was a problem hiding this comment.
Suggested change
| override fun onDestroy() { | |
| super.onDestroy() | |
| _binding = null | |
| } | |
| override fun onDestroyView() { | |
| _binding = null | |
| super.onDestroyView() | |
| } |
- Fragment에서 객체의 생명주기와 View의 생명주기는 서로 다릅니다
- super.onDestroyView는 부모의 클래스 소멸자 함수이기에 이후에 호출하는 로직들은 예상치 못하는 결과를 발생시킬 수 있습니다(이걸 개발할 때 Side Effect라고 부릅니다). 그래서 먼저 로직을 실행하고 super.onDestroyView를 호출하는 것이 더 좋습니다.
| ?.add(R.id.container_rv, followerFragment)?.commit() | ||
|
|
||
| binding.btnFollower.setOnClickListener { | ||
| getActivity()?.supportFragmentManager?.beginTransaction() |
There was a problem hiding this comment.
Suggested change
| getActivity()?.supportFragmentManager?.beginTransaction() | |
| requireActivity()supportFragmentManager.beginTransaction() |
이렇게 하면 nullable하지 않습니다.
Contributor
Author
There was a problem hiding this comment.
많이 늦었지만 알려주신 것대로 코드 모두 수정했습니다! 덕분에 몰랐던 지식을 많이 얻어갑니다 :) 코드리뷰 정말 감사합니다!!!
mdb1217
approved these changes
Dec 22, 2021
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
week7 과제
까지 구현했습니다~