AuthManager code review

[codeofmochi]

This is a code review for the AuthManager implementation.

Styling

Don't forget to style your code and remove your unused packages. You can simply use the automated linter in Android Studio by pressing Ctrl+Alt+o (optimize imports) and Ctrl+alt+l (format code) in every file.

Packages

Try using more packages. As you create more files, your main package will become overcrowded and it will be difficult to find specific bits of code. You could modularize your files hierarchy, for instance :

ch.epfl.sweng.studdybuddy/
    models/
        Account.java
        // your other model classes here
    providers/
        AuthManager.java
        FirebaseAuthManager.java
        OnLoginCallback.java
        // any other external wrapper such as database accesses
    activities/
        // your android activities here

Using Android Studio's integrated refactoring will help as it will automatically resolve all paths (right-click on a file -> refactor -> move).

Account class

https://github.com/geofot96/StudyBuddy/blob/65460e9e3ad33ed8c8922041da378cd5341a0bb4/app/src/main/java/ch/epfl/sweng/studdybuddy/Account.java#L7-L9

Is your class supposed to be immutable? If yes, don't forget to add the final keyword.

https://github.com/geofot96/StudyBuddy/blob/65460e9e3ad33ed8c8922041da378cd5341a0bb4/app/src/main/java/ch/epfl/sweng/studdybuddy/Account.java#L16

What is this empty constructor used for ? The constructed object would not be very useful since you can't get any existing property.

AuthManager

https://github.com/geofot96/StudyBuddy/blob/65460e9e3ad33ed8c8922041da378cd5341a0bb4/app/src/main/java/ch/epfl/sweng/studdybuddy/AuthManager.java#L6-L11

The abstraction is good. It could be more consistent by using Tasks only or callbacks only rather than mixing the 2 (this helps if another developer is using the interface). Read further below.

FirebaseAuthManager

https://github.com/geofot96/StudyBuddy/blob/65460e9e3ad33ed8c8922041da378cd5341a0bb4/app/src/main/java/ch/epfl/sweng/studdybuddy/FirebaseAuthManager.java#L25-L32

You don't need to pass clientID as a String argument. You can simply add another line to retrieve the string from the context :

    this.ctx = currentActivity;
    String clientID = ctx.getResources().getString(R.string.default_web_client_id)
    GoogleSignInOptions gso = ... ;

This is in fact better as it hides another implementation detail from the caller.

OnCompleteListenerWrapper

https://github.com/geofot96/StudyBuddy/blob/65460e9e3ad33ed8c8922041da378cd5341a0bb4/app/src/main/java/ch/epfl/sweng/studdybuddy/FirebaseAuthManager.java#L35

https://github.com/geofot96/StudyBuddy/blob/65460e9e3ad33ed8c8922041da378cd5341a0bb4/app/src/main/java/ch/epfl/sweng/studdybuddy/OnCompleteListenerWrapper.java#L9-L33

I don't really understand the motive behind this class. From what I've understood, you want to define what happens after the user has logged in, so that would be related to the user interface and the activities. But your new class does not help this, as you have no access to the context calling the login process. Besides, you would want to define your UI logic in the activities itself. Why not return the task returned by signInWithCredential directly so that you can handle the resolution on call site ? Your API would also be more consistent as login would also return a task and you can even ditch the passed callback. You can then also drop the TAG string parameter (btw this also makes more sense since this is an activity specific property, it should not leave its scope).

So you would change the signature of your implementation (and your interface) :

    public Task<Void> login(Account acct)

Now you can change your implementation by simply returning the login task :

    public Task<Void> login(Account acct) {
        AuthCredential credential = GoogleAuthProvider.getCredential(acct.getIdToken(), null);
        return mAuth.signInWithCredential(credential)
    }

You can then delete your OnCompleteListenerWrapper.java and resolve the task on site (in your calling activity) :

  authManager.logout().addOnCompleteListener(new OnCompleteListener() {
    @Override
    public void onComplete(Task<TResult> task) {
      if (task.isSuccessful()) {
          // update UI, which is possible since we are in the caller activity's context
      } else {
          // handle failure, again easy because we are in UI context
      }
    }
  })

DummyGoogleSignInActivity and DummyMainActivity

I think you have misunderstood the usage of dummy classes. You should only implement dummy classes to swap implementations that are not testable. By creating these dummy activities (aka UI controllers) you actually introduce more issues :

• this breaks the separation of concerns : you now have logic definitions for your AuthManager inside UI files
• you now have files that should not be part of the final system that live at the same level as your production files (they get compiled into your product)
• you only use them for rewriting the auth manager
• so now basically you don't really test your actual UI but rather another controller that "replaces" your real UI. But this is actually counter-productive : what you originally wanted was to keep the exact same UI class (so that you can still black box test it and its interactions) even if you can't test other parts of the system such as the authentication system.

These 2 files (dummy activities) should not exist at all. The only dummy class should exist in the test environment only, and should swap at runtime against the untestable code (FirebaseAuthManager) that only describes logic (no UI interaction).

What you want is a DummyAuthManager, defined in your test package, that you swap at runtime when you're instantiating your activities' instrumented tests. That way, you can still test the UI without the need to test the Firebase code and the dummy code never leaves the test environment. This DummyAuthManager will contain contain trivial implementations for your AuthManager interface contract (for instance login and logout would return a resolved task, startLoginScreen would call the tested activity's onActivityResult and getCurrentUser could return some default Account).

So in your androidTest package, you could have a definition that looks like this (I use TaskCompletionSource) to emulate the Task result:

public class DummyAuthManager implements AuthManager {

    private Activity context;

    public DummyAuthManager(Activity act) {
        this.context = act;
    }

    public Task<Void> login() {
        TaskCompletionSource<Void> tcs = new TaskCompletionSource<>();
        tcs.setResult(null); // maybe you'll need to delay it, if this doesn't work see google API
        return tcs.getTask();
    }

    public Task<Void> logout() {
        // same thing
    }

    public void startLoginScreen() {
        // you should put the dummy test code here
        context.onActivityResult(1, 0, null);
    }

    public void getCurrentUser() {
        return new Account("John Doe", "testid", "testtoken");
    }
}

Now when you instantiate your tests, you could swap the implementation in the before rule : your instrumented tests should now look like this :

@RunWith(AndroidJunit4.class)
public class YourClassActivityTest {
    @Rule
    public ActivityTestRule<YourClassActivity> rule = new ActivityTestRule<>(ActivityTestRule.class);

    // swap implementation when the tests start
    @Before
    public void beforeEachTest() {
           AuthManager dummy = new DummyAuthManager(rule.getActivity());
           rule.getActivity().setAuthManager(dummy);
    }
}

MainActivity and GoogleSignInActivity

https://github.com/geofot96/StudyBuddy/blob/65460e9e3ad33ed8c8922041da378cd5341a0bb4/app/src/main/java/ch/epfl/sweng/studdybuddy/MainActivity.java#L55-L60

https://github.com/geofot96/StudyBuddy/blob/65460e9e3ad33ed8c8922041da378cd5341a0bb4/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java#L100-L105

It looks like you are trying to implement some sort of Singleton pattern. However, this does not apply here as you're directly controlling the instantiation of the object (recall that the Singleton pattern is useful when the object itself wants to manage its own instances). Why not use basic instantiation and dependency injection ?

This could become :

public class YourClassActivity extends AppCompatActivity {
  private AuthManager auth; // unfortunately we can't instantiate here, because we're missing the context

  @Override
  public void onCreate(Bundle savedInstanceState) {
    ...
    auth = new FirebaseAuthManager(this);
    ...
  }

  public void setAuthManager(AuthManager auth) {
    this.auth = auth;
  }
}

https://github.com/geofot96/StudyBuddy/blob/65460e9e3ad33ed8c8922041da378cd5341a0bb4/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java#L107-L109

This code should not exist here. There should be no test related code inside the production code. I see that you are calling this function several times to change the actions if the activity is in test mode or not. But why not abstract everything so that swapping the AuthManager instance takes care of it for you ? Hence you won't need any testing logic inside your UI logic (which is exactly what you want).

https://github.com/geofot96/StudyBuddy/blob/f787ecc7bf6abf51df60020dd6c405200aa298ad/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java#L32-L36

Same goes for this code, but now that you have reimplemented startLoginScreen, you don't even need to test this condition anymore. Just call the function.

https://github.com/geofot96/StudyBuddy/blob/f787ecc7bf6abf51df60020dd6c405200aa298ad/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java#L51

this instead of GoogleSignInActivity makes more sense as this is not a static property, we are in the same scope.

https://github.com/geofot96/StudyBuddy/blob/e27314dde8759a2968733b0ba0d78cee0daeb031/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java#L63-L81

Wouldn't it make more sense for this part to be part of the FirebaseAuthManager ? I mean that's debatable. Part of me thinks that these are implementation details that can be part of the concrete login() implementation (hence you would also avoid rewriting all of this if you decide to implement user login elsewhere). You could argue that these are not part of Firebase authentication itself, but I'd say that the GoogleSignIn.getSignedInAccountFromIntent(data) call cannot be tested anyway, so I think it would be better to abstract all of this out into the login() function. But you would need to call login with the Intent data instead of an Account. I believe it still makes more sense, or you could add a loginFromIntent method to the interface and concrete classes. However these test related parts :

https://github.com/geofot96/StudyBuddy/blob/e27314dde8759a2968733b0ba0d78cee0daeb031/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java#L64

https://github.com/geofot96/StudyBuddy/blob/e27314dde8759a2968733b0ba0d78cee0daeb031/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java#L68

https://github.com/geofot96/StudyBuddy/blob/e27314dde8759a2968733b0ba0d78cee0daeb031/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java#L96-L98

These should not exist either. That's why it's superior to put the whole previous bit about GoogleSignInAccount into login. Swapping the instance will do that for you, you really should not have any test related code in your controller.

GoogleSignInWrapper

https://github.com/geofot96/StudyBuddy/blob/65460e9e3ad33ed8c8922041da378cd5341a0bb4/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInWrapper.java#L17-L29

I also don't understand why is this wrapper needed. As mentionned before, shouldn't this be part of the FirebaseAuthManager login method or with the rest of the code of onActivityResult ?