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 - Mindcraft #189

Open
codeofmochi opened this issue Dec 10, 2018 · 0 comments
Open

Code review - Mindcraft #189

codeofmochi opened this issue Dec 10, 2018 · 0 comments
Assignees

Comments

@codeofmochi
Copy link

This is a code review for the code of the whole project on master branch at commit
https://github.com/Claudio5/SDP/tree/e1dbebc870cf5934c8fac0fbf55fc615906b4baf

Of course, we don't expect you to change all of your code (some parts of the review can be subjective or arguable anyway), important is that you understand why better alternatives exist.

Please ask us if you have any question about the code review, whether it is by writing a comment below or by sending us an email or meeting us.

Important : if you try to fix these, don't break your app. I prefer that the app works even if you don't manage to fix anything. It may be too hard to rework many of the issues below, as it would require to rewrite the whole architecture.

Globally, your code is pretty clean and well-documented. You make use of several design patterns, some are well thought, others don't always seem like the best option. You have a few architectural issues with your code, so please read this review carefully.

Packages

You make good use of packages with regular naming conventions. Your choice of separating the packages semantically is reasonable. Sometimes it may make more sense to split the models (classes holding data), the views (such as your activities), the providers (external libraries) and the utility classes into their respective package so that all developers know exactly where is each type of code (although currently it's fine in your project).

Provider abstractions

There is no abstraction between your classes and the external libraries you use, such as Firebase, Auth, Functions or Glide. Currently, you directly use these proprietary implementations at the call site (i.e. every time you access the database from your activities or load an image into a view). This is a huge problem for maintanibility, separation of concerns and ease of use :

  • First, all the developers need to know the internal workings of Firebase to be able to access the database (the developers team cannot scale).
  • This also places database implementation details into other parts of the code that may be completely unrelated to the database such as views and data models (there is no longer a separation of concerns and semantics between such classes and the database stuff).
  • Maintainability becomes a pain : if you change the implementation of your database (maybe you rearrange the structure or need to upgrade the API), then you need to touch a lot of different files to fix all accesses to the DB. Even worse : now imagine that your product owner suddenly decides that you need to use a relational database or a graph-oriented database instead of Firebase (maybe for cost or performance reasons). Then you need to remove all the parts where Firebase is called and replace them with the new implementation (the code cannot scale).
  • Finally, the lack of abstraction makes it very hard to describe your database. We would want to express what to do with the db with ideas, not rewrite how it works every time.
  • The same issues apply for any of your external dependency, like Glide. Auth should also be a separate abstraction.

We want to be able to simply call database.getRooms() and you retrieve a List<Room> for instance, or database.getUser(id) and you get a single User (btw these can be sync or async) without worrying about the implementation details. The solution to this is regrouping common library logic into a single class : you could have an interface Database (respectively ImageLoader) that only describes the actions you want to perform on the database (resp. images) without any implementation logic. For instance :

public interface Database {
    // i'm using futures here but you could also have callbacks for Java < 8
    public Future<User> getUserByName(String username);
    public Future<Room> getRoomById(String roomId);
    ... // you get the idea
}

public interface ImageLoader {
    public <S> void display(S source, ImageView view, List<BitmapTransformation> transforms);
}

Then, you could have concrete definitions for these interfaces that actually implement the proprietary logic (it may make sense to use singletons).

public class FirebaseDatabase implements Database {
    FirebaseDatabase db = FirebaseDatabase.getInstance();

    public Future<User> getUserByName(String username) {
        Future<User> f = new CompletableFuture<User>();
        db.getReference("users").orderByChild("username").equalTo(username)
            .addListenerForSingleValueEvent(new ValueEventListener() {
                @Override
                public void onDataChange(@NonNull DataSnapshot snapshot) {
                    f.complete(snapshot.getValue(User.class))
                }
                ...
            })
        return f;
    }

    ... // all your other database accesses defined here
}

The cool thing with Futures is that you can resolve them on call-site and then do whatever you want with your data. Also btw, Firebase has a convenient function on DataSnapshot called getValue(class) in which you can pass the class of your model (for instance here User.class) and it will convert it automatically without the need to manually convert the <String, Object> map (your cloneAccountFromFirebase method for instance).

Finally, you instantiate these providers once (for instance in your home activity you will have a line where Database database = new FirebaseDatabase() and ImageLoader imageLoader = new GlideImageLoader()) and then pass them around using dependeny injection. Each of your fragment / activity will receive a Database (resp. ImageLoader) object (which underlying instance is a FirebaseDatabase but you dont need to know it when you use it, just call the methods), and you can perform your actions without writing any implementation logic at call site : for instance you would only call database.getUserByName("johndoe") and poof you get a User without the need to rewrite all the frills from Firebase.

This is very nice, because let's say you want to suddenly change to a relational database now : then you only need to create a single new SQLDatabase class which implements Database. Then, instead of instantiating a FirebaseDatabase, you create a SQLDatabase. Voilà, no other changes and it works out of the box. The same procedure applies to ImageLoader and your other libraries.

Models

Immutability

You should be familiar with the concept of immutability. Your classes are mutable (which is not a problem by itself, this is more of an open discussion). However, using immutability would allow you to get rid of state which often eliminates many nasty bugs and maintenance issues. Of course, this is a trade-off against higher memory usage and more garbage collection.

The fields of an immutable class must have the final keyword. If they are referenced properties, you need to deep copy and freeze them when you construct the class and also when they are retrieved with a getter (for instance lists and objects must be rebuilt field by field). Also, since many of these are data classes, it may be useful to re-implement equals, clone and the like, so that you can treat theses classes the same as values.

Many of your models could actually already benefit from immutability (such as your shop items, leagues, ...) : indeed you construct them and you only define getters on them, so you can already get rid of state and prevent misusage of these classes. Of course, this does not apply to models that necessarily need state (such as your Account class).

Waiting rooms

Could Room be a model ? I see that most of the implementation is directly done on Firebase fields in Matchmaker and in your activities, but in my opinion it would make sense that Room is a concept defined in a class, and you could for instance create a room, room.join() and room.leave(). Of course, the implementation of these methods should use the Database abstraction I wrote about earlier. (This is just an idea to openly discuss, it simply stuck me as I was reading these code parts, you don't need to implement it).

Classes

Account.java

Singleton implementation

I understand that the singleton pattern is necessary (and a good idea!) because you need it everywhere and it models the current user once. However your current implementation has this chicken and egg problem that you have factories to create the instance externally (createAccount()) because you need to instantiate the class with parameters from outside of the class : this defeats the whole purpose of the singleton pattern since the class does not hold control over its instances anymore (proof is that you need a runtime IllegalStateException in your constructor : this is for sure a code smell).

So from my understanding, you actually fusionned 2 concepts together and this creates many issues in your code : the Account model, and the account of the current user (let's call it CurrentAccount).

  • Account is a representation of any user account and should only hold fields related to the concept of Account (userId, username, email, currentLeague, ...) but NOT implementation details as to where it lives (no database references or handlers). And this class doesn't need to be a singleton right? It can represent any account, hence if you instantiate another Account that's not a big deal, it's not part of the application state (there's no need to protect a particular instance). This will also allow you to get rid of the context parameter in getInstance() (which makes more sense IMO).

  • CurrentAccount on the other hand, will be a singleton on behalf of the current user's account, which needs to be unique and protected from being instantiated again. It will only hold an instance of Account, and will only define getInstance which will either spawn the instance from the distant / local database (this is where I would put the logic to differentiate where to create the instance from, not from the external activity) if this is the first call to the method, or return the previously created one.

Arguably, you could also make Account an immutable class, and have CurrentAccount update the instance with a new Account for each new state of the Account class (however this adds memory / performance overhead and it may be overkill, this isn't the best use case anyway).

So in pseudo-code :

public class Account {
    private String userId;
    private String username;
    private String email;
    private String currentLeague; // can't this be strongly typed ? You have a League class right ?
    ... // no DB stuff though

    public Account(...) {
        ...
    }

    // getter, setter, methods, ... 
}

public class CurrentAccount {
    private final Account instance = null;
    
    private CurrentAccount() {} // no construction allowed

    /* 
        We assume Database and LocalDB have completely different interfaces 
        and Auth is an abstraction for authentication. Here they all are
        singletons and synchronous (assumptions to simplify my example)
    */
    public static Account getInstance() {
        if (instance == null) {
            // pseudo-code to differentiate whether we fetch from online or local
            Auth auth = Auth.getInstance();
            if (auth.isLoggedIn()) {
                instance = Database.getInstance().getAccount(auth.getUserId());
            } else {
                instance = LocalDB.getInstance().retrieveAccount(auth.getUserId());
            }
        }
        return instance;
    }
}

This would allow you to create the account and save it (either online or offline). Then, as soon as you call CurrentAccount.getInstance(), you know that the account exists and that it is valid.

Lastly, I admit this may not be the most optimal solution (dependency injection would do the job much better), but given Android's terrible way of handling object passing between activities, it is at least safer than relying on exceptions.

Database everywhere

The other main issue in this class is that you have database logic everywhere (as explained above), while Account should be a data class simply holding state for the app. The Database abstraction would help you clean all of these. This shows up in all your methods that need to access the DB (changeTrophies, updateCurrentLeague, updateItemsBought, ...). Especially you have one method that is completely unrelated to the Account class (checkForDatabaseException) and also the createCompletionListener.

AccountCreationActivity.java

  • Glide and Firebase logic in your classe. For instance, if you had the DB abstraction you would only call something like database.createNewUser(Account, onAlreadyExist, onSuccess).

ConstantsWrapper.java

  • I don't really understand this class. Doesn't it provide the same thing as Database for getReference? An Auth abstraction could contain getUserId that would fetch the Firebase ID internally and manage the login state instead of getFirebaseUserId here.

LoginActivity.java

  • The EMAIL constant is used as extra identifier that you pass to AccountCreationActivity. However in AccountCreationActivity you retrieve it with the same hardcoded string "email", which means that if you need to change this identifier some day you have to change it twice. I agree that the extra passing mechanism is horrible design from the Android ecosystem, but in my opinion it would make more sense to have these extra identifiers defined either at the sender activity or the receiving activity (put pick one and stick to it everytime you pass an extra) as a public static final string. That way you always have the same convention and always know where the identifier is defined, and it also becomes much more maintainable.
  • Firebase logic in handleSuccessfulSignIn.

UsernameInputWatcher.java

  • Cool use of TextWatcher
  • What's getString doing here? Shouldn't it be part of an UI utility class if you have similar definitions elsewhere ? In fact I don't think this method is too useful, you could just call resources.getString(id) directly (I'm being picky here 😋).

Database.java

  • As explained earlier, your current Database.java does not abstract the database from the implementation : it exposes all the internals to the rest of the app (the references, the proprietary types, and so on). It's a good thing that you have tried to centralize the calls into a single class, but your current implementation only forwards calls without providing an interface.
  • Why is this an enum? This is a terrible implementation for a singleton and will confuse developers that are new to the codebase. Either your class must be unique and holds state => then implement a proper singleton. Otherwise, it should be an uninstantiable class with public static methods only (like a utility class). Hence, you trick the compiler by declaring an INSTANCE that isn't used anywhere : remember that enums should represent types that can be mapped to integer values.
  • The DatabaseReferenceBuilder is cool, but it should be an internal detail that is only used inside the FirebaseDatabase class, it should not be exposed to the outside.

FbStorage.java

  • This class is much better on the other hand, using a utility static class pattern. Your sendBitmapToFirebaseStroage method does provide some kind of abstraction, but this is not enough (in my opinion) : we still need to pass a proprietary StorageReference as parameter, and it returns a StorageTask. Why not make it so that FbStorage itself handles the logic of keeping track of the StorageReferences and wrap the StorageTask with a common object such as an Optional or by using callbacks as parameter? Then you would only expose the functionality through carefully defined methods (ideally inheriting from a Storage interface for example).

Item.java

  • Nice and clean implementation. If I'm not mistaken, I believe this class (and its children) can already be made immutable since you don't change their state afterwards (maybe I'm wrong here).

All children of Item.java (SlowdownItem.java, SpeedupItem.java, ...)

  • What purpose do the factories (create...Item) serve? They simply return a new instance without any additional logic, so shouldn't you just make the constructors public and get rid of the factories ?

Items.java

  • This looks very convoluted : shouldn't the enum just store all possibilities? Since getRandomItem and getRandomItemForOfflineMode are only used in RandomItemGenerator, wouldn't it make more sense that these methods are defined there instead (together with your lists, sizes, random) ? This would better separate different logic.

DrawingActivity.java

  • Shouldn't createColorImageView and clear(View view) be inside some View utility class ? (this is an implementation choice, but if you redefine them somewhere else then you definitely should)

DrawingOnline.java, DrawingOffline.java, DrawingOnlineItems.java

  • You should keep the -Activity suffix in the class name, this makes naming more consistent and understandable. The semantic is completely lost in DrawingOnlineItems. However, your hierarchy from GyroDrawingActivity with each sub-Activity adding its required functionalities is very nice.
  • Internal and external database logic inside the activities

LoadingScreenActivity.java

  • Glide, Firebase logic inside activity

  • What's that cast for? It can simply be task.getException().printStackTrace() right? line 147 :

    if (exception instanceof FirebaseFunctionsException) {
        FirebaseFunctionsException ffe = (FirebaseFunctionsException) exception;
        ffe.printStackTrace();
    }

    If I understand correctly, there's no other error handling if something goes wrong ? Maybe it would make sense to go back to the main screen or something.

RankingFragment.java

  • You have an empty constructor and you still chose to define extras. Why don't you enjoy the fact that you can finally pass your objects in a typed manner through the constructor for once (thanks Android 😁)? (new RankingFragment(roomID,drawings,playersNames))
  • Firebase logic inside fragment

VotingPageActivity.java

  • Firebase and Glide logic inside activity
  • What are the methods at the bottom used for? They forward the exact some call to another function for testing, but isn't the point of @VisibleForTesting to be able to show private methods to the testing framework?

WaitingPageActivity.java

  • Firebase logic
  • Why duplicate animateWord1 and animateWord2 ? Since they do the exact same thing, can't they be merged into a single parameterized function ?

GameResult.java

  • In my personal opinion, I think that the model of GameResult should be separate from its conversion into a view (it could be a separate view-model class).

HomeActivity.java

  • Firebase and AuthUI logic

LeaderboardActivity.java

  • Shouldn't Player be a class by itself? I would also split view logic from the model class.
  • Same for Leaderboard. Also firebase logic.
  • FriendsButton could be moved into its own class in a UI package.

League.java

  • The factories are unnecessary : you just forward the creation of the object, you could get rid of them and use the constructor directly. Also why are the 3 factories doing basically the same thing with different parameters? It seems redundant to me : the issue comes from the fact that LayoutUtils basically holds the logic for your Leagues management. I guess you wanted to prevent the creation of other leagues, but instead you should have a LeagueManager that handles that.

localDatabase package

  • Your local database wrappers are much nicer that their online counterpart. There you have better abstraction from the methods you have defined. The only issue now is that your local DBs are not generic : if we needed to change the underlying local storage technology, you would need to change every part of the code where you instantiate them. You could make them more generic, first by creating interfaces containing only the bare minimum functions that describe the actions you want to perform (for instance the first interface would only have saveAccount, retrieveAccount). Arguably, you could regroup the 3 of them inside a single class (although that would be a huge class), but this would allow you to pass a LocalDatabase reference around using dependecy injection, then you can call your functionalities through localdb.yourMethod().

MatchMaker.java

  • Why is account passed as a dependecy to Matchmaker? Wasn't it defined as a singleton? (I know this is caused by the context dependency, but from my previous advice you won't need it anymore)
  • FirebaseFunctions and FirebaseDatabase logic

Shop.java

  • Firebase logic : it would make more sense that the Database abstraction fetches a List<ShopItem> directly through some Database.getShopItems() for instance.

ShopItem.java

  • I would separate the ShopItem from its conversion to view logic.
  • I think this class can be immutable by dropping your setters that are used only for tests anyway, and you only have getters left. The only setter left is setOwned, but I would argue that it makes sense to reverse the implementation : the Player owns ShopItems, not the other way around.

BooleanVariableListener.java

Activity.java

  • Bad naming choice, there's already an android.app.Activity, you may confuse new developers that are used to the Android ecosystem. Should be named BaseActivity, and BaseActivity should be named NoBackPressActivity instead.

MainActivity.java

  • Firebase logic

General stuff

  • I like your checkPrecondition function (this is good defensive programming!). Advanced advice : keep in mind that performance is still affected (not a lot) : sometimes you may want to use a contract framework (such as CoFoJa) and run it only in your debug builds. Then the production release may not need all checks (but your current implementation is fine, it's already far more reliable).

  • You can auto-format and optimize your imports automatically using Android Studio.

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

7 participants