Code review - Human Learning

[codeofmochi]

# Code review

This is a code review for the code of the whole project on main branch at commit fdae25e1b1

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

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

Important : if you try to fix these, don't break your app. We prefer that the app works even if you don't manage to fix anything. However, we still expect you to refactor your code as much as possible.

General remarks

Packages structure

In general your packages are nicely structured and there is a clear hierarchy defined.

You may introduce subpackages for interfaces, so as to further elaborate the hierarchy, i.e. one subpackage for the interfaces and one for the implementing classes (often called interfaces and impl). It seems weird to have your database abstractions located in the model package, consider adding a providers or services top-level package and moving these files there, and in which you could then have implementation subpackages interfaces, firestore, and offline for instance.

Documentation

We noticed that a lot of classes and methods lack documentation. We do require you to properly document all methods and classes. This holds for every team member. Even UI classes, which you consistently do not comment on, will greatly benefit from comments, as they are difficult to read and understand.

We therefore had some trouble understanding your code and evaluating its implementation as without formal specification of what you’re trying to accomplish, it is very difficult to verify if it adheres to it.

All public classes, methods and fields must be documented. Use KDoc conventions: a top-level block for general description and metadata, and per-method header with annotations (@param, @return, @throws, ...). You can omit KDoc on overriden methods if they are themselves documented. Regarding the implementation itself, of course you shouldn’t comment on each line of code, but rather only explain sections of code that cannot be understood just by reading the code with the help of a relevant comment.

Code style

It seems you don’t use the integrated auto-formatter. You can automatically format all of your code by right-clicking on the com.github.HumanLearning20201.HumanLearningApp package and hitting “Reformat code” and “Optimize imports”, and with Ctrl+Alt+L and Ctrl+Alt+O on a per-file basis. Ideally, you should do it at least every time you finish a feature, before requiring a merge to the main branch.

In particular, you seem to very often exceed the default wrapping limit of 80 columns, which makes your code very “wide”. You also have inconsistent use of spaces and indentation, as well as many unused imports. Use the auto-formatter and import optimizer to ensure sane conventions across your team, as well as all Android developers (for instance if a new developer was to join the team in the future).

You often don’t use the functional features of Kotlin. This makes your code much more verbose (for instance you often rebuild mutable collections to filter them, instead of simply calling .filter with a predicate). Kotlin is heavily inspired by Scala, so you should remember from your functional programming classes that these features will help you reduce boilerplate and chain operations using function composition. The collections API is very similar to Scala’s.

Code quality & performance

Leverage your IDE to do the hard work for you: you should also use the integrated lint issues detection and code analysis by using Analyze > Inspect Code > Whole project. Some reported problems may not actually be important, however it seems you have many Kotlin warnings. Make sure to fix as many of those lints as possible.

For instance, you have some instances of redundant null checks and nullable return types that can be converted to strict types. It is a good thing that you practice defensive programming, however here the compiler will guarantee stricter rules about the soundness of your types to be enforced, such as non-nullable types that can never be null.

It seems you use exceptions in a lot of places in your code. In Kotlin, this is an anti-pattern (see https://kotlinlang.org/docs/exceptions.html) and you should avoid relying on them to express potential return outcomes from functions. Instead, you should use nullable types (if your error can be summarized with the Option={Some|None} pattern) or the Result monad if more complex cases are needed. Note also that try/catch is a valued expression in Kotlin and not a statement.

In particular, since you use coroutines which are dispatched as asynchronous operations, it is a bad idea to mix synchronous constructs (such as exceptions) with asynchronous execution. By using the return value instead as an indicator of potential failure, you ensure that you obtain any possible outcome at the expected return time (i.e. when the coroutine yields back to the main thread).

You also use a lot of non-null assertions (!!) that may crash the app. Consider using the smart features of null safety in Kotlin and prefer using the Elvis operator ?., ?.let checks and ?.mapNotNull and similar for collections. Also, you should adopt a convention on when to use nullable types, and when to ensure null safe types (you have inconsistent return types regarding nulls within each file).

Class hierarchy

You define your models using interfaces: in particular, they are all defined as interfaces containing vals to be overridden. This is very common in structural subtyping languages (such as Typescript or Python), however this does not scale well with nominal subtyping based languages, which Java and Kotlin belong to.

Indeed, you redefine the shape of the objects for each specific underlying use case: for instance, interface Category is overridden by concrete implementations FirestoreCategory, DummyCategory and OfflineCategory. First, this way of writing code generates a lot of files and boilerplate (we can see all your models are duplicated in each service package). It also means that your implementation of business logic is never independent: it makes it difficult to reason about with domain-specific code, and changing providers would be a pain: imagine your client in the future decides that you should not use Google products anymore, you would hence need to rewrite all your data classes.

In nominally-typed languages, it is better to Keep your models Simple Stupid. As such, entities are mostly modeled using Plain Old Java/Kotlin Object (POJO / POKO), and this makes sense as you want your data to have an instantiable representation which you can manipulate purely with your language of choice. This makes your business logic independent from the rest of your systems (UI, storage, …). If you need an extended representation for storage, consider using the Adapter or Decorator pattern instead. Also, using plain objects will remove the need for your DummyX classes, which will become the actual models themselves. We strongly recommend you implement your models using Kotlin data classes.

Note that this problem would not appear in a structurally-typed language: since the compiler would inspect the exact shape of the object and its properties, you wouldn’t need to explicitly redefine the relationship between your “core” object (i.e. the mathematical representation of your entity stripped of any implementation-specific property) and the extended objects that are actually stored on the underlying medium. This is why you may often see models defined as interfaces in e.g. Typescript, but rarely in Kotlin or Java. In those languages, interfaces usually model behaviour, while (abstract) classes model state.

Classes

DatabaseManagement and DatabaseService

These 2 interfaces are extremely similar, and each have concrete implementations. We don’t really understand their purpose: it seems they are huge interfaces to define access methods to the database.

Both interfaces are extremely similar: in fact, almost all signatures are identical, and DatabaseService defines a few more to manage users. This is a nasty code smell: it either means you duplicate functionalities, or that the boundaries of your modules are not clearly defined and overlap. We can see that in all your concrete implementations of DatabaseManagement, you mostly forward the calls to the corresponding instance of DatabaseService, and we do not understand why you add this overhead. To us it reads like an indirection that doesn’t provide abstraction.

It also feels like these interfaces are becoming “god classes” which will contain anything related to data access. Although this is fine for the scope of your project, it will not scale to dozens or hundreds of entity types. You may want to have a look at the Repository pattern, which groups accesses per entity or aggregate of entities, see https://docs.microsoft.com/en-us/dotnet/architecture/microservices/microservice-ddd-cqrs-patterns/infrastructure-persistence-layer-design, which in your case would group for instance picture accesses into one Repository, datasets into another, etc...

We argue that the interface/class name DatabaseManagement is not semantically precise, and seems to highlight the issue we explained above. If you name something a Manager, it probably means that either its purpose is too vague, or that a better name can be found. Have a look at this article: https://blog.codinghorror.com/i-shall-call-it-somethingmanager/

In your DatabaseService, your method updateUser takes a FirebaseUser as input: this should not happen in an interface, because it closely couples the behaviour to the proprietary types from Firebase. It should take a User instead. In general, interfaces should allow you to swap the underlying systems very easily as it is their goal to abstract the implementation details away.

Otherwise, the interfaces are well documented and you did a good job abstracting the access to Firebase. This is the kind of documentation that we would like you to provide throughout your code.

FirestoreDatabaseManagement

• No KDoc
• See comment about exceptions, try-catch and coroutines in section “Code quality” above
• See comment about DatabaseManagement and DatabaseService above: it seems you mostly delegate calls to databaseService. This class is probably useless.
• Your concrete Firestore implementation of DatabaseManagement should not require the passed arguments of model type X to your methods to be a FirestoreX: indeed, this is not what the contract you established with the interface says. For instance, your interface dictates that if you pass a Category to getPicture, you should get back a CategorizedPicture. The line require(category is FirestoreCategory) violates this contract: it means that the logic that performs the conversion between a Category and a FirestoreCategory is not at the right place. Instead it should be the role of the Repository itself to perform the conversion (using an Adapter, Decorator, extended class or similar). This holds true for every method in which you force a downcast using require(x is FirestoreX).
• You have the same issue for your return types: you should not return a FirestoreX model as your contract is to return a model of type X. The Repository should again perform the conversion back to a POKO, and the rest of your application should be completely unaware of your adapted types. This affects all your return types.
• getCategoryByName should probably be named getCategoriesByName since you return a collection. You can simplify the whole method to this single line: return databaseService.getCategories().filter { it.name == categoryName}.
• getDatasetByName should probably be named getDatasetsByName since you return a collection. You can simplify the whole method to this single line: return databaseService.getDatasets().filter { it.name == datasetName }.
• getDatasetNames and getDatsetIds: you perform a copy using a mutable set, however there is no benefit in deep-copying an immutable collection. You can return the set directly without copying it.
• removeCategoryFromDataset: it seems weird that this method in the data access layer modifies the core model directly and then returns it. In our opinion, it should be your business logic’s responsibility to ensure that the model was modified beforehand.

FirestoreDatabaseService

• No KDoc
• See comments from file above and in “Code quality”: in particular about require downcasts and return types
• getDatabaseNames in your companion object completely leaks the Firebase implementation details to the external world and your whole app: indeed you access it through FirestoreDatbaseService.getDatabaseNames, and it returns low-level technicalities that shouldn’t concern anything else than the Firestore repository. Besides, you only forward it to UniqueDatabaseManagement.getDatabases, which in turn is only used in a single test. You should probably completely remove this method.
• Regarding your internal schema classes (CategorySchema, PictureSchema, ...), it seems they subsume their corresponding Firestore classes (FirestoreCategory, FirestorePicture). From our understanding, it probably means that the latter Firestore classes are redundant: your schemas already handle the internals and conversions to the Firestore representation, and it does it better than your Firestore classes because they keep everything hidden from the outside world. Consider completely removing your FirestoreX data classes and keep your schemas to use them as Adapters.
• Consider making all your schemas private inner class to be able to access the members of the FirestoreDatabaseService instance.
• Your DatasetSchema.toPublic implementation is very convoluted: you probably don’t need the ExperimentalStdlibApi, you use a mutable set instead of performing immutable transformation, and your null-check is too early in the call chain. Consider replacing the whole val cats = ... expression with val cats = this@FirestoreDatabaseService.categories.get().await().toObjects(CategorySchema::class.java).mapNotNull { it.toPublic() }.toSet().
• getCategories does not need the experimental buildSet: use return categories.get().await().toObjects(CategorySchema::class.java).mapNotNull { it.toPublic() }.toSet() instead
• getAllPictures does not need the experimental buildSet: transform the method as above. Also, see comment in “code quality” about exceptions in Kotlin, as well as the one about require downcasts for the previous file.
• In general, for all your methods in this file, read carefully the previous comments, as well as the “code quality” section above about exceptions in Kotlin, as well as the one about require downcasts for the previous file. Many of the issues mentioned until now apply here.
• putDataset: see comment in code quality about non-null assertions.
• getDatasets: simplify with function collections API as above

CachedFirestoreDatabaseManagement

• No KDoc
• This class is a decorator for a FirestoreDatabaseManagement, however it could probably be a decorator for any DatabaseManagement since there is no specific Firestore logic. However, consider refactoring by merging it with part of the DatabaseService into a CachedPictureRepository, as well as redesigning your models into POKOs (see comments in section “class hierarchy”).
• This also means that cachedPictures should not hold values of type FirestoreCategorizedPicture but instead CategorizedPicture
• You can replace your ?: return null statements by simply applying .let instead. For instance replace val fPic = … putIntoCache(fPic) by db.getPicture(pictureId)?.let{ putIntoCache(it) }.

FirestoreCategorizedPicture

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This class is probably redundant.
• Function displayOn is not the correct place: this is a UI concern, and a model should contain business logic only. Consider writing a UI utility function in the view package display(pic: CategorizedPicture, ctx: Context, imageView: ImageView) instead.

FirestoreCategory

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This class is probably redundant.

FirestoreDataset

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This class is probably redundant.

FirestoreDocument

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This interface should instead be a package-private abstract class from which your schemas inherit.

FirestoreUser

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This class is probably redundant.

modules

• Your dependency injection seems very convoluted. While it is great that you use a DI framework, you should minimize the indirection such that you only provide real dependencies for the production system. For test mocks, you should be able to inject them manually in your tests by constructing the objects by yourself (such as putting a mock database in your presenters for instance). You shouldn’t need to run any code in the dependency injection declarations.

CategorizedPicture

• No enough KDoc: use @property
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This interface is redundant.
• Function displayOn is not the correct place: this is a UI concern, and a model should contain business logic only. Consider writing a UI utility function in the view package display(pic: CategorizedPicture, ctx: Context, imageView: ImageView) instead.

Category

• No enough KDoc: use @property
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This interface is redundant.

Converters

• No KDoc
• This object will become useless if you address our comments from DatabaseManagement and DatabaseService.
• In Kotlin (as in Scala), an object construct compiles to a singleton with a memory footprint. If you simply want to namespace static functions (to be available statically, such as Java’s public static method), use package-level functions.

Dataset

• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This interface is redundant.

DummyCategorizedPicture

• No enough KDoc: use @property
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This “dummy” class should actually become your POKO model that represents this entity in your app logic.
• Function displayOn is not the correct place: this is a UI concern, and a model should contain business logic only. Consider writing a UI utility function in the view package display(pic: CategorizedPicture, ctx: Context, imageView: ImageView) instead.

DummyCategory

• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This “dummy” class should actually become your POKO model that represents this entity in your app logic.
• Function displayOn is not the correct place: this is a UI concern, and a model should contain business logic only. Consider writing a UI utility function in the view package display(pic: CategorizedPicture, ctx: Context, imageView: ImageView) instead.

DummyDatabaseManagement

• No KDoc
• See all comments from FirestoreDatabaseManagement. This class is redundant, and probably useless.

DummyDatabaseService

• Not enough KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”.
• What is the end purpose of this class? If it’s meant to stay as a default content provider (i.e. stuff that appear on the app even without any persistence or server) then it makes sense to refactor it as a DefaultContentRepository and either create a Repository that aggregates this content with the remote one, or directly combine both repository calls in the app logic itself. Otherwise, if it’s only meant to be test data (that should not be visible in production), then move this class to the tests package.

DummyDataset

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This “dummy” class should actually become your POKO model that represents this entity in your app logic.

DummyUser

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This “dummy” class should actually become your POKO model that represents this entity in your app logic.

UniqueDatabaseManagement

• No KDoc
• We could not understand the purpose of this class. From what we’ve read, it seems like a very complicated way to initialize your databases all at once in your module loader through Hilt. We could not figure out much of your code, but here are few things that really seem odd to us:
• It seems your offline mechanism relies on downloading entire databases, and keeping track of these “meta-entities”. While this may be useful for complex systems with versioning, it seems over-engineered for the scale of your app. Wouldn’t it make more sense to download the entities themselves and simply implement some synchronization mechanism?
• It seems you use this class to switch between your offline / test / local / firestore implementations at module loading time. This seems like a lot of responsibilities that can be implemented in a simpler way. For test mocks, you should manually inject them. For offline / local caching, you could simply use decorators (especially if you refactor considering the comments in “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement” about repositories). In all other cases, your app logic should handle runtime cases.
• You have a lot of code that seems to run at module initialization time. While not necessarily wrong, we do not really understand why so much has to be executed at injection.

User

• Not enough KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This interface is redundant.
• Your enum class Type seems a bit weird: it leaks implementation details regarding the concrete instance of the User, but this is related to your problems of interface and redundant adapters.

CachePictureRepository

• No KDoc
• We guess this class is WIP. Otherwise it provides useless indirection and you should then remove it.

OfflineCategorizedPicture

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This class is probably redundant.
• Function displayOn is not the correct place: this is a UI concern, and a model should contain business logic only. Consider writing a UI utility function in the view package display(pic: CategorizedPicture, ctx: Context, imageView: ImageView) instead.

OfflineCategory

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This class is probably redundant.

OfflineConverters

• No KDoc
• This object will become useless if you address our comments from DatabaseManagement and DatabaseService.
• In Kotlin (as in Scala), an object construct compiles to a singleton with a memory footprint. If you simply want to namespace static functions (to be available statically, such as Java’s public static method), use package-level functions.

OfflineDatabaseManagement

• No KDoc
• See comment about exceptions, try-catch and coroutines in section “Code quality” above
• See comment about DatabaseManagement and DatabaseService above: it seems you mostly delegate calls to databaseService. This class is probably useless.
• You have the exact same problems as in FirestoreDatabaseManagement. The fact that the code is almost exactly a duplicate shows a severe code smell. By addressing the Repository structure we proposed, this class should probably become a Decorator for your Firestore repository instead.
• As such, you won’t need complex logic to inject this dependency

OfflineDatabaseService

• Not enough KDoc
• See comment about exceptions, try-catch and coroutines in section “Code quality” above
• See comment about repositories: this class would become an offline repository, which you could implement as a decorator for any other repository kind (so from the top-level Repository interface), and which would perform offline caching on top of it.
• You have a type issue for your return types: you should not return a OfflineX model as your contract is to return a model of type X. The Repository should again perform the conversion back to a POKO, and the rest of your application should be completely unaware of your adapted types. It seems indeed weird that your “putX” methods pass the model as argument, and return a specified version of it afterwards. This affects all your return types.
• You also have useless copies of data that are already immutable.

OfflineDataset

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This class is probably redundant.

OfflineUser

• No KDoc
• See comments in section “Code quality”, “FirebaseDatabaseService” and “FirebaseDatabaseManagement”. This class is probably redundant.

PictureRepository

• No KDoc
• See comment about exceptions, try-catch and coroutines in section “Code quality” above
• @Throws is not a Kotlin annotation: it is there to support legacy checked runtime exceptions for Java interoperability.
• This class is probably misnamed: it is not a repository because you never return entities. It is simply a file accessor for your image cache.
• You have a direct access to the proprietary Firebase.storage instance in savePicture. This line is completely breaking the abstraction by doing so: the purpose of your interfaces and dependency injection is to avoid using the concrete implementations. You want to describe the behavior, and let the system provide you with the actual resource. This would allow you for instance to swap Firebase for another provider very easily, otherwise you would need to change all of your files.

AuthenticationPresenter

• No KDoc
• A presenter should not be a singleton, and should not persist if the corresponding views are destroyed. Instead, it should be the other way around: each view instantiates a presenter, which will hold the models (see a possible implementation here), however you rightfully understand that the data is lost across activities. Thus to manage your shared data you should use a repository if you decide to keep the MVP pattern. Otherwise, have a look at the ViewModel pattern, which solves this problem by allowing you to bind one ViewModel to many Views and thus share state: https://developer.android.com/topic/libraries/architecture/viewmodel
• You use proprietary types AuthUI in the constructor, as well as Firebase directly in several methods: this makes your code closely coupled to Google services and is thus not maintainable. For instance, imagine that your customer decides to avoid Google products for privacy reasons then you would have to rewrite all your proprietary calls. Instead, consider abstracting authentication behind an AuthProvider (similarly to how Repositories abstract data access). You may want to hold a modeled representation of your current user in the AuthProvider itself (which would make sense to implement as a singleton).

LearningPresenter

• Missing class-level KDoc
• Your @Demo2Database annotation is very cryptic. Does that mean you inject mocks into production? Also what do you use custom annotations for?
• The instantiation of your presenter here seems more correct: you simply inject it into one Fragment
• Your learningMode, dataset and previousCategory are global variables when the presenter is instantiated: you should never directly mutate state from a View. In MVP, all actions should be performed through a contract interface (i.e. methods), and you should keep the state private to the presenter. In fact, you probably don’t even need Hilt here: you could initialize the LearningPresenter manually in the LearningFragment at method onCreateView and pass those values as constructor parameters (since you only assign them here anyway).

CrossRefs

• No KDoc
• See comments about DatabaseManagement, DatabaseService and their implementations. In particular, none of the parameter or return types should be specific to Room, but rather return your POKO models

Daos

• No KDoc

Entities

• No KDoc

Relations

• No KDoc

RoomOfflineDatabase

• No KDoc

RoomTypeConverters

• No KDoc

AddPictureFragment

• No KDoc
• It does not really make sense to have _binding hold a nullable binding and redefine the binding getter to forcefully assert non-null. Why not simply let it a nullable and check at every use site, or otherwise use a lateinit var?
• For instance in onCreateView you could juste return _binding?.root instead of duplicating

CategoriesEditingFragment

• No KDoc
• removeView: it seems weird to assume that the passed parameter will always have a parent on which you can find your EditText: you should specify through types or documentation. Also, you may want to replace your for / if / break with a functional expression such as filter.

DatasetsOverviewFragment

• No KDoc
• Same useless non-null assertion as above with binding

DisplayDatasetFragment

• No KDoc
• Same useless non-null assertion as above with binding

DisplayImageFragment

• No KDoc
• Same useless non-null assertion as above with binding

DisplayImageSetFragment

• No KDoc
• Same useless non-null assertion as above with binding

SelectPictureFragment

• No KDoc
• Same useless non-null assertion as above with binding
• It seems weird to have view IDs that end with some seemingly random digits
• As you have commented, you should fix the deprecation (don’t suppress the lint issue if it’s something that you plan to fix)
• displayPicture: you may want to abstract your picture loader behind an interface, so that you can easily swap to another implementation if needed

TakePictureFragment

• No KDoc
• Same useless non-null assertion as above with binding
• Nicely modularized through functions

DatasetListRecyclerViewAdapter

• No KDoc

DatasetListWidget

• Not enough KDoc
• It is good practice to keep the type suffix from which the class inherits from (i.e. you may want to name it DatasetListFragment).

LearningAudioFeedback

• Not enough KDoc

LearningDatasetSelectionFragment

• No KDoc
• Same useless non-null assertion as above with binding

LearningFragment

• No KDoc
• It is good practice to keep the type suffix from which the class inherits from (i.e. you may want to name it GoogleSignInFragment).

LearningSettingsFragment

• No KDoc

GoogleSignInWidget

• No KDoc
• Hardcoded string in updateUi

HomeFragment

• No KDoc

MainActivity

• No KDoc

Testing

• Your testing code should be as readable as your production code
• All tests should meaningfully assert conditions or check useful properties

Conclusion

Overall, your code has several flaws that make it hard to read and to maintain. Although you had some great ideas for abstracting and modularizing your system, their implementations are clunky which in the end do not solve the complexity problem.

For instance, you should probably refactor your models so that their definitions are data classes and not interfaces, since Kotlin is not structurally sub-typed. Otherwise, this generates a lot of duplication and boilerplate. Similarly, although your database interfaces describe the behavior nicely, you have a confusion between your so-called “DatabaseService” and “DatabaseManagement”, which you could solve more elegantly with the Repository pattern. Many problems result from these design decisions, such as type leakages and contract violations and class roles duplication.

You should also attempt to simplify your dependency injection: it should lessen the burden on the programmer for binding dependencies. In general, abstractions are great but they should only be used if they can provide a new meaningful name to a concept and hide implementation details.

Another example is many code snippets can be simplified through the use of the functional API readily available on the Kotlin collections. Null safety and exceptions are also completely different from Java, so you may want to read a bit more about them in the Kotlin docs.

As a general guideline, try to read a bit more examples for instance from the Android documentation, as well as the practices for the environment you work in (each framework and each language has its own quirks and ways to do things). You may also find out that many of the problems that you face have already been solved through libraries, design patterns or recommended best practices.

Please make a habit to write documentation as you write code: this will also help your teammates in getting familiar with the whole codebase. Also, make sure to be thorough in your code reviews: when you write or review code, you must take responsibility for it, i.e. try to put yourself in the shoes of someone else that has never seen your code before: is it readable? Is it easy to reason about? Are modules self-contained? Which classes should handle what? Try to draw clear boundaries between your functionalities.

Let us know if you have any questions or remarks with the comments below.

[bbjubjub2494]

In particular, since you use coroutines which are dispatched as asynchronous operations, it is a bad idea to mix synchronous constructs (such as exceptions) with asynchronous execution. By using the return value instead as an indicator of potential failure, you ensure that you obtain any possible outcome at the expected return time (i.e. when the coroutine yields back to the main thread).

I take issue with the statement that exceptions are synchronous constructs in the specific context of Kotlin coroutines: according to the official documentation, exceptions can be used in coroutines by design, and they will propagate to the semantic caller of the function (or the parent coroutine if applicable). This also reflected in our experience with the framework: we never had exceptions leak out of the coroutine and we were able to reliably catch them upstack.

Of course you presented other reasons to use a pseudomonad which we will consider.

[Nitwix]

Thanks for the feedback! It already seemed to me that DatabaseService & DatabaseManagement didn't really make lots of sense to have separately. We will probably refactor this if the team decides it is worth the time it will take. The suggestion to change interfaces to data classes where applicable also makes a lot of sense to me, and won't take too long to refactor I think. Regarding the use of functional APIs, I don't know if we'll take the time to change everything in the code that we have already written, but we will certainly try to use it more in the future.

Would you be ok if we do a refactoring sprint in the future, where we mostly focus on refactoring ? Or does this go against the Scrum principles ?

[jonaskon]

It seems your offline mechanism relies on downloading entire databases, and keeping track of these “meta-entities”. While this may be useful for complex systems with versioning, it seems over-engineered for the scale of your app. Wouldn’t it make more sense to download the entities themselves and simply implement some synchronization mechanism?

I do not really understand this comment, we wanted to download an entire database by design. The objective is to be able to give a user the possibility to learn entirely offline (i.e the same way you would download a Spotify playlist) and the best way I found to do so was using the Room library. Do you mean that it would have been better to do it differently?

[jonaskon]

Some of the (very valid) comments about the whole DatabaseService, DatabaseManagement... structure have been addressed by the refactoring done in last weeks sprint (pull request #138) I believe.

[codeofmochi]

I take issue with the statement that exceptions are synchronous constructs in the specific context of Kotlin coroutines: according to the official documentation, exceptions can be used in coroutines by design, and they will propagate to the semantic caller of the function (or the parent coroutine if applicable). This also reflected in our experience with the framework: we never had exceptions leak out of the coroutine and we were able to reliably catch them upstack.

This is true, however there are subtleties regarding the exact handling mechanism. See https://kotlinlang.org/docs/exception-handling.html. Of course it is very possible to rely on exceptions in Kotlin, at the end it's a matter of choice and consistency. As a side note, functional advocates sometimes consider exceptions to be the reincarnation of goto as they make it very easy to break control flow. The idea is that return types render error handling explicit, so as to avoid stacktrace hell while maintaining the project as the code grows. Kotlin doesn't support checked exceptions for that reason, and generally reserves this mechanism for runtime failures that cannot be caught at compile time.

Would you be ok if we do a refactoring sprint in the future, where we mostly focus on refactoring ? Or does this go against the Scrum principles ?

As you have guessed, with Scrum you should deliver new features every sprint (generally the customer does not see refactors, and he may not even understand the value behind them). One great way to explain them in that scenario is to show that time and money will be saved in the future by doing so. For this course, we recommend you pick a few new light features while refactoring.

I do not really understand this comment, we wanted to download an entire database by design. The objective is to be able to give a user the possibility to learn entirely offline (i.e the same way you would download a Spotify playlist) and the best way I found to do so was using the Room library. Do you mean that it would have been better to do it differently?

This is a valid approach and there's nothing inherently wrong with it. We simply wanted to suggest a simpler alternative as your code to handle that is relatively complex.

Let us know if you have any more questions and keep up the good work :slightly_smiling_face: