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

[둘리] 3, 4단계 오목 제출합니다. #34

Merged
merged 55 commits into from
Mar 29, 2023

Conversation

hyemdooly
Copy link

안녕하세요, 리뷰어님!
2단계 피드백을 참고해서 반영했고 안드로이드로 구현했습니다.
피드백 부탁드립니다! :)

woowahan-pjs and others added 27 commits March 25, 2023 01:20
@hyemdooly hyemdooly force-pushed the step4 branch 2 times, most recently from 222a93c to 8bedfaa Compare March 24, 2023 16:32
…삭제, 뷰 설정 코드 메인 액티비티로 이동, DBHelper에 함수 추가
@hyemdooly
Copy link
Author

액티비티가 MVC에서 컨트롤러의 역할이라고 생각했는데, 막상 작성해보니 액티비티를 뷰로 봐야하는지 컨트롤러로 봐야하는지 혼란이 생겼습니다!
현재 코드는 액티비티가 뷰의 역할도, 컨트롤러의 역할도 하고 있는 것 같습니다.
뷰의 역할을 하는 다른 클래스를 작성하기에는 액티비티에서 모든 뷰를 넘겨줘야하고, 액티비티에 이렇게 모든 코드를 넣기에는 경계가 모호해지는 것 같습니다.
어떻게 작성하는게 옳은 방법인지 잘 모르겠습니다.🥲

@laco-dev
Copy link

액티비티가 MVC에서 컨트롤러의 역할이라고 생각했는데, 막상 작성해보니 액티비티를 뷰로 봐야하는지 컨트롤러로 봐야하는지 혼란이 생겼습니다!
현재 코드는 액티비티가 뷰의 역할도, 컨트롤러의 역할도 하고 있는 것 같습니다.
뷰의 역할을 하는 다른 클래스를 작성하기에는 액티비티에서 모든 뷰를 넘겨줘야하고, 액티비티에 이렇게 모든 코드를 넣기에는 경계가 모호해지는 것 같습니다.
어떻게 작성하는게 옳은 방법인지 잘 모르겠습니다.🥲

MVC 패턴에서 액티비티의 역할은 무엇일지를 잘 찾아보셨나요?
지금 가지고 계신 고민은 안드로이드 MVC 패턴이 가지는 문제점을 잘 이해하신 것입니다.

이후 MVP 패턴 MVVM 패턴으로 이어지면서 현재 구조가 가지는 문제점에 대해 학습 해보시면 됩니다.

Copy link

@laco-dev laco-dev 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 14 to 17
init {
val latestPlayer = blackPlayer.getLatestPlayer(whitePlayer)
_players = Players(latestPlayer, listOf(blackPlayer, whitePlayer))
}

Choose a reason for hiding this comment

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

latestPlayer를 계산하기 위해 Player.getLatestPlayer(player) 를 호출하는 흐름이 어색하게 느껴집니다.

그렇다 보니 각 플레이어 구현체에 동일한 로직이 중복해서 발생하였는데요.
턴을 결정하는 것은 Players가 하고 있으니 마찬가지로 Players에서 처리 해보면 어떨까요?

val players = Players.from(blackPlayer, whitePlayer)

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 40 to 46
var result: TurnResult = TurnResult.Playing(false, omok.players)
boards.forEachIndexed { index, view ->
view.setOnClickListener {
if (result !is TurnResult.Playing) return@setOnClickListener
result = omok.takeTurn(calculateIndexToPoint(index))
onEndTurn(view, index, result)
onEndGame(result)

Choose a reason for hiding this comment

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

실제 게임을 진행하는 로직도 onCreate()에서 같이 분리 해보면 좋을 것 같아요!

더 고도화로 간다면 리스너에서 외부의 상태를 변경하는 형태라면 인터페이스를 구현해서 상태를 내부로 캡슐화 시키는 방법도 있습니다
아래는 반영하실 필요는 없고 참고만 해주세요.

class BoardClickListener(index: Int, omok: Omok, onEndTurn: (), onEndGame: ()) {
   private var result: TurnResult = ...
   override fun onClick(...)
}
class BoardPlayManager(...) {
   fun start() {
      turn = ...
      onStartGame()
   }
}

Comment on lines 62 to 63
val blackIndexs = dbHelper.getIndexsByColor(StoneColor.BLACK.name)
val whiteIndexs = dbHelper.getIndexsByColor(StoneColor.WHITE.name)

Choose a reason for hiding this comment

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

DB 헬퍼를 raw값으로만 주고받도록 잘 만들어 주셨네요.

다만 이렇게 하게 되면 디비에 넣는 값이 변경되거나 스펙이 수정되면 코드를 외부에서 수정하는 상황이 생기기도 합니다.

이미 이름 자체가 오목 데이터베이스 헬퍼이기 때문에 오목에 대한 도메인을 가져도 된다고 생각합니다.

whiteIndexs.forEach {
setStone(boardViews[it], StoneColor.WHITE)
private fun onEndTurn(view: ImageView, index: Int, result: TurnResult) {
val context = applicationContext

Choose a reason for hiding this comment

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

MainActivity는 Context 타입이랍니다 😮

Comment on lines 21 to 25
if (endTurnPlayers == _players) return TurnResult.Playing(true, _players)
_players = endTurnPlayers
return TurnResult.Success(_players)
if (_players.isFoul) return TurnResult.Foul(_players.curPlayerColor, _players)
if (_players.isPlaying) return TurnResult.Playing(false, _players)
return TurnResult.Win(_players.curPlayerColor.next(), _players)

Choose a reason for hiding this comment

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

if문이 반복해서 호출되면 when 을 사용 해보면 어떨까요?

Copy link

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

안드로이드 환경에서 구현하더라도 지금까지 해온 미션을 잘 살릴 수 있도록 잘 활용 해보시면 좋겠습니다.
오목 미션은 종료하겠습니다.

그동안 고생 많으셨습니다. 🎉

@laco-dev laco-dev merged commit 50e7c2f into woowacourse:hyemdooly Mar 29, 2023
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.

None yet

3 participants