이원석, 21.10.17 #10
이원석, 21.10.17 #10
Conversation
코드리뷰 요청 - FirebaseLoginManager.cs 의 33 ~ 39 Line의 문제점 수정 - 이전에 추천해주신 로그인 관련 클래스 구성의 구현에서 현재 구현한 것이 말하신 방법인지 모르겠음 - 코드의 가독성 향상 클래스 관계 LoginManager : FirebaseLoginManager와 OAuthloginManager을 합쳐서 관리함 FirebaseLoginManager : Firebase관련 로그인 기능 구현 OAuthLoginManager : OAuth관련 로그인 기능 구현 TryLogin : 로그인을 시도하는 클래스 PrintLog : 모바일 화면에서 Text UI로 로그를 찍어주는 역할
There was a problem hiding this comment.
전체적으로 이전에 코드를 봤을 때에 비해 불필요한 부분이 많이 줄어들었습니다.
그러나 아직 유니티의 메인 스레드 시스템에 대해 지식이 부족한 부분(FirebaseLoginManager, Line 33~39)이 있고, 3항 연산자로 간단히 처리되는 부분을 if-else 문으로 꼭 분리를 했어야했나, 하는 생각도 듭니다.
또한 조언을 급하게 받아들이느라 제대로 이해하지 못했거나 조언에 따라 작업하다 중간에 불필요한 작업 또는 클래스가 작성되는 경우가 있습니다. 이는 조언을 한번 곱씹어보고 설계에 대해 한번씩 생각해보면 도움이 될 것 같습니다.
| get => _logText.text; | ||
| set => _logText.text = $"{value}\n"; | ||
| } | ||
|
|
| @@ -0,0 +1,38 @@ | |||
| using UnityEngine; | |||
|
|
|||
| public class TryLogin : MonoBehaviour | |||
There was a problem hiding this comment.
'TryXXX' 는 특정 동작을 하되 실패시 예외를 던지지 않는 메소드에 바람직한 네이밍입니다.
아예 Login 과 같이 직관적인 이름으로 바꾸는 것이 낫습니다.
| private void CheckLoginSuccess(bool success) | ||
| { | ||
| PrintLog.instance.LogString += "Start CheckLoginSuccess"; | ||
| if (success) |
There was a problem hiding this comment.
해당 if-else문은 다음과 같이 줄일 수 있을 것으로 보입니다.
PrintLog.instance.LogString += success ? $"[Login Success]" : $"[Login Failed]";| get => _logText.text; | ||
| set => _logText.text = $"{value}\n"; | ||
| } | ||
|
|
There was a problem hiding this comment.
LogString을 쓸 때 마다 instance 를 붙이는게 귀찮다면 다음과 같이 메소드화 시키면서 LogString 프로퍼티의 set 요소를 없앨 수 있습니다.
public static void AddLog(string str)
{
instance._logText.text = $"{str}\n";
}
...cs
PrintLog.AddLog("디버그 실패");| if (success) | ||
| { | ||
| AuthCode = | ||
| PlayGamesPlatform.Instance.GetServerAuthCode(); |
There was a problem hiding this comment.
줄바꿈을 할 이유가 없어보입니다. 나중에 정리해주세요.
|
|
||
| UnityEngine.Social.localUser.Authenticate((bool success) => | ||
| { | ||
| if (success) |
There was a problem hiding this comment.
해당 if-else문은 다음과 같이 줄일 수 있을 것 같습니다.
AuthCode = success ? PlayGamesPlatform.Instance.GetServerAuthCode() : "";
PrintLog.instance.LogString += success ? "[Google Login Success]" : "[Google Login Failed] : success is false";| /// nextAction : Authenticate 완료 시 실행할 함수. bool 매개변수는 Authenticate 작업의 성공 여부를 뜻함. | ||
| /// </summary> | ||
| /// <param name="nextAction"></param> | ||
| public void AuthenticateOAuth(System.Action<bool> nextAction) |
There was a problem hiding this comment.
보통 콜백으로 붙이는 메소드의 경우 특정한 상황에 실행되기 때문에 'OnXXX' 라는 식으로 네이밍을 합니다.
| @@ -0,0 +1,38 @@ | |||
| using UnityEngine; | |||
|
|
|||
| public class TryLogin : MonoBehaviour | |||
There was a problem hiding this comment.
해당 클래스의 역할 자체에 의문이 있는데, LoginManager 가 있는데 굳이 또 로그인을 하는 오브젝트를 만들어주는 이유가 있나요? 전부 LoginManager 에서 해결하면 문제가 생길나요?
돌려보지 않은 상태라서 문제가 생기는지는 모르나, 로그인을 하는 객체도 있고 로그인 매니저라는 객체도 있으면 둘의 역할이 불분명해지고 헷갈릴 것 같습니다.
| // 아래의 부분을 호출하지 않음 | ||
| // 그러나 Firebase Console을 확인해 보면 계정이 성공적으로 추가되어있음 | ||
| User = task.Result; | ||
| nextAction(task.IsCompleted); |
There was a problem hiding this comment.
해당 부분이 호출이 안되는건 아니고, 해당 태스크는 유니티 메인 스레드에 위치해있지 않은데 Unity API에 접근했기 때문에 호출이 되지 않은 것처럼 보이는 것입니다.
정확히는 작동은 했으나, 유니티 메인 스레드가 아닌 곳에서 실행되는 nextAction 에서 Unity API를 호출하기 때문에 API가 반응하지 않은 것이고, 파이어베이스에는 정상적으로 작동된 것입니다.
Potato Thief 프로젝트 코드리뷰 요청
클래스 관계 및 설명
LoginManager : FirebaseLoginManager와 OAuthloginManager을 합쳐서 관리함
FirebaseLoginManager : Firebase관련 로그인 기능 구현
OAuthLoginManager : OAuth관련 로그인 기능 구현
TryLogin : 로그인을 시도하는 클래스
PrintLog : 모바일 화면에서 Text UI로 로그를 찍어주는 역할