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

부산대 Android_노신 2주차 과제 Step2 #32

Open
wants to merge 23 commits into
base: normenghub
Choose a base branch
from

Conversation

Normenghub
Copy link

코드 작성하면서 어려웠던 점

가능한 MVVM을 사용하라 하였지만 구현을 못했습니다. 다음 주차 과제 진행시 공부를 더 해서 적용해볼려고 합니다.

또한 저번 과제에서는 커밋을 함수 단위로 쪼개서 많은 커밋들이 생성 되었습니다.

이번에는 기능 단위로 쪼갤려고 노력을 했는데 searchView와 RecyclerView 그리고 DB 연동을 한번에 커밋하였는데 약간 크게 커밋한거같아서 잘못한 것 같은 느낌이 들었습니다.

중점적으로 리뷰해주셨으면 하는 부분

  1. 변수 SCOPE 적절한지.
  2. 클래스를 잘 나눴는지
  3. 접근 제한자 사용을 잘 하였는지
  4. 위젯에서 아쉬운 부분이 있는지
  5. 커밋단위를 적절하게 쪼갰는지
    알고 싶습니다. !

Copy link

@LeeOhHyung LeeOhHyung left a comment

Choose a reason for hiding this comment

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

@Normenghub 2주차 step2 과제도 고생많으셨습니다. 코멘트 한번 확인부탁드립니다.

Comment on lines 57 to 66
override fun onQueryTextChange(newText: String?): Boolean {
newText?.let {
if (it.isEmpty()) {
showNoResultMessage()
adapter.updateData(emptyList())
} else {
searchPlaces(it)
}
}
return true

Choose a reason for hiding this comment

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

newText?.let { ... } 으로 scope function을 사용해도 되지만, String.isNullOrEmpty() 메소드를 제공해주고 있습니다. 이것으로 if - else 조건문을 바로 사용할 수 있을것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

확인 했습니다!

Comment on lines +101 to +107
private fun getPlacesFromDatabase(query: String): List<Place> {
return databaseHelper.getAllData().filter {
it.name.contains(query, ignoreCase = true) ||
it.address.contains(query, ignoreCase = true) ||
it.category.contains(query, ignoreCase = true)
}
}

Choose a reason for hiding this comment

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

p2;

  • DB에 있는 모든 컬럼을 조회한후에 원하는 데이터를 필터링 하는것 보다 쿼리를 날릴때 WHERE, LIKE 조건을 추가해서 원하는 결과만 리턴받도록 하면 더 좋을것 같습니다. SQL 문법으로 작성한다면,LIKE '%query%' 같은 조건이 추가되겠네요. 커서쿼리 방식으로도 가능한 방법이 있는듯 하네요 (StackOverFlow)
  • SQLiteDb 클래스는 데이터베이스와 관련된 역할수행을 담당하고 있습니다. 그래서, SELECT 쿼리를 날리는 메소드도 SQLiteDb 에서 수행되는게 조금더 적절해보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

확인 했습니다!

Comment on lines +8 to +9

class PlacesAdapter(private var places: List<Place>, private val onItemClick: (String) -> Unit) : RecyclerView.Adapter<PlacesAdapter.PlaceViewHolder>() {

Choose a reason for hiding this comment

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

p3; PlacesAdapter를 생성할때 listof() 로 빈 리스트를 전달해주는데요.
클래스 멤버변수로 ArrayList 타입의 places를 변수를 가지고있고, 업데이트가 필요할때는 addAll 메소드로 이미 선언했던 리스트에 데이터를 삽입해주도록 하는게 조금더 좋을것 같습니다.

class PlacesAdapter (...) {
  private val places = arrayListOf<Place>()

  fun updateData(newPlaces: List<Place>) {
    places.addAll(newPlaces)
    notifyDataSetChanged()
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

확인 했습니다!

Comment on lines +81 to +87
private fun insertData() {
val dbHelper = SQLiteDb(this)
for (i in 1..10) {
dbHelper.insertData("카페 $i", "서울 성동구 성수동 $i", "카페")
dbHelper.insertData("약국 $i", "서울 강남구 대치동 $i", "약국")
}
}

Choose a reason for hiding this comment

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

p4; 데이터베이스에 미리 데이터를 넣어두는 것을 SQLiteDb의 init { } 블록 or SQLiteDbHelper의 onCreate에서 수행하는것도 방법일것 같습니다. sqlite pre populate data 라는 키워드로 검색해보면 관련 내용들을 찾아볼 수 있을듯합니다. 혹시 여유가 되신다면 한번 도전해보세요!

override fun onCreate(db: SQLiteDatabase) {
    // ... 데이터베이스 스키마 설정

    // Insert initial data
    val insertData = ("INSERT INTO $TABLE_NAME ($COLUMN_NAME, $COLUMN_AGE) VALUES "
            + "('John Doe', 30), "
            + "('Jane Doe', 25), "
            + "('Sam Smith', 22)")
    db.execSQL(insertData)
}

Choose a reason for hiding this comment

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

table1이 카페/약국 정보를 저장하기 위한 테이블이고, table2가 검색어 기록을 위한 테이블이네요. 데이터베이스에 미리 설정된 값을 넣어둔다면 table도 기록을 위한목적 하나만 사용가능해보이네요.

Copy link
Author

Choose a reason for hiding this comment

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

확인 했습니다!

Comment on lines +39 to +44
historyAdapter = HistoryAdapter(historyList.toMutableList()) { id ->
val deletedRows = databaseHelper.deleteFromSelectedData(id)
if (deletedRows > 0) {
historyAdapter.removeItemById(id)
} else { }
}

Choose a reason for hiding this comment

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

p3; 변수를 선언할때는 어떤 의미를 담고있는지 최대한 나타내주는것이 좋습니다.
가령 isDeleteSelectData 처럼 "데이터베이스에서 데이터를 제거하는데에 성공했다" 라는 의미가 잘 전달되도록 deletedRows > 0 을 서술적으로 풀어주는것이죠.

Copy link
Author

Choose a reason for hiding this comment

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

확인 했습니다!

Comment on lines +29 to +37
databaseHelper = SQLiteDb(this)
recyclerView.layoutManager = LinearLayoutManager(this)
adapter = PlacesAdapter(listOf()) { name ->
databaseHelper.insertIntoSelectedData(name)
updateHistory()
}
recyclerView.adapter = adapter

historyRecyclerView.layoutManager = LinearLayoutManager(this, LinearLayoutManager.HORIZONTAL, false)

Choose a reason for hiding this comment

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

p4; 지연초기화로 필요할때 클래스를 대입해주는것도 괜찮지만, 클래스가 생성되는 시점에 만들어도 되는 클래스까지 lateinit var 을 사용할 필요까지는 없습니다. lateinit도 결국 mutable variable이기때문에 변경가능하다는 취약점이 있고, 바뀌지 않는다면 클래스 멤버변수들이 초기화 될때 인스턴스가 생성되어도 괜찮을것 같습니다. 가령 SQLiteDb 나 PlacesAdapter, HistoryAdapter 같은 클래스들이 예시가 되겠네요.

지연초기화 방법에는 by lazy 라는것도 있으니, 차이점을 비교하면서 사용해보시면 더 좋을것 같습니다

Choose a reason for hiding this comment

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

XML에 app:layoutManager="..." 속성을 사용했다면, Activity에서 LayoutManager를 설정해주지 않더라도 inflate될때 자동으로 설정되게 됩니다. 따라서, recyclerView, historyRecyclerView에 layoutManager를 설정해주는건 중복코드가 될것 같네요

Copy link
Author

Choose a reason for hiding this comment

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

확인 했습니다!

@Normenghub Normenghub requested a review from LeeOhHyung July 7, 2024 11:54
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.

2 participants