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

Code Review By Team2 #26

Open
onteeae opened this issue Nov 21, 2018 · 0 comments
Open

Code Review By Team2 #26

onteeae opened this issue Nov 21, 2018 · 0 comments

Comments

@onteeae
Copy link

onteeae commented Nov 21, 2018

Team4

Backend:

  1. Test
    (저희도 현재 그렇게 하고 있지만)
    Response Code로 확인보다는 Mock Object로 바꾸는게 좋을 것 같아요.
  2. Arbeit.views
    Serializer를 사용하는 게 좋을 것 같습니다.
  3. User를 왜 이 방식으로 구현하셨는지?
    User Key를 가지는 것 보다 Basic User를 상속받아서 구현하셔야 했을 것 같아요
  4. Null-True Field들이 많은데,
    이러면 DB상에서 메모리/디스크 효율 문제가 있을 것 같아서 차라리 Detail Model을 하나 만들어서 FK로 물고있는게 낫지 않을까요?

Front:

  1. Module
    Module을 좀 나누어서 계층구조를 만드는 게 좋을 듯.
  2. EvenEmitter등 사용해서 app을 좀 나누는 게 좋을 것 같아요
    (App 하나에 너무 많은 기능을 몰아넣은 느낌)
  3. 하드 코딩된 Value들 구현 하셔야 할 듯
  4. 0,1,2,3 이런 식으로 control하시는데, Enum등을 선언 해서 구현하는 것이 나을 것 같습니다.
  5. arbeit-post-create에서, is_finished 같은 경우는 custom validator등으로 대체하면 좋을 것 같습니다.
  6. arbeit-post-create make_temp_post 이것도, 각 region/arbeit_type을 확인할 때
    html에 enum을 bounding해서 표시되는걸 {{model.name}}이런 식으로 수정하시면
    if elseif elseif …. else를 안하셔도 될 것 같습니다.
  7. arbeit-bulletin.html같은 경우도 Filtering Tag 하드 코딩 보다는, Enum이나 List등을 만들어서 ngFor로 렌더링하는게 더 좋아보입니다.(확장성 고려)
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

No branches or pull requests

1 participant