Neighborfood Code review

# Neighborfood Code review # General Remark Hello team ! As promised, we take a look at your code. As pointed out almost everytime during the meetings and the grade feedbacks, you did almost no code reviews. Since you told us everytime that you struggle to find something to say, we were expecting to have not much to say (recall that we also suggest you to see code reviews from other teams several times). Unfortunately, the reality is the other way around. By looking at your code base, we saw that the lack of communication in your teams make you do inconsistencies. Some of you use some conventions, other uses differents ones. Often, you use many useless inheritance hierachy making your code really hard to understand. Over the top, there is almost nothing commented... Below, you will find a detailled code review of the whole codebase. Almost all the issues we detected could have been prevented by doing proper code reviews and having more communication between you. We think that all the issues should be solved in addition of your usual sprint work as this is work that should have be done properly in the first place (so this should be done before next friday, in addition to a properly planned sprint (that you should have send us by Monday the 25th)) and we also give you one week to fix all your problems. Also, we feel that, sometimes, you limit yourselves to the strict necessary. By that we mean that you work enough, but often your task are "finished" but there are still some TODO's. This is not a correct behaviour regarding your teammates (See for example the `return null;` in the `fetchAll` method in the database, or the `try-catch` in `VendorProfileActivity` class, but there are plenty others and we won't list all of them here). Yeah, it will happen that sometimes you'll spend much more than the required time, but it is also an organisation problem as you run out of time to finish your tasks properly. We can point you to the following Sweng lectures that might help you preventing the issues again - [Good code](https://docs.google.com/document/d/1fzIDk-r4W1L_A8ac0dTwJB4VtrP5p5aX6mr4TtUgLMI/edit) - [Code reviews](https://docs.google.com/document/d/1G3PLrJA2DK2DdsjqqzCBa5qvVvm90ycugGxy0-tks-w/edit#heading=h.ew44on5sb2b) ## Code structure The structure of the code is not too bad. The package are well classified. Nevertheless, you could have put all the `Factory` objects in a single package. ## Code style Globally, here are the most repeated issues: - The comments are clearly insufficient, both in terms of JavaDoc and code comments. As you will see this is the most recurrent issue and it appears in almost all the class. The comments should absolutely be done at the end of the project as this is a really important aspect of the software engineer job. - (a bit subjective but) Clases contain a lot of useless empty spaces and (less subjective) useless imports. You should use `ctrl+alt+l` (for the indentation) and `ctrl+alt+o` (clean useless importants and variables) from time to time to clean a bit your code (on each file). - You should agree on whether or not to use lambda functions or anonymous classes and to be consistent throughout the code. We suggest you to go for the lambdas as they provide cleaner and readable code. - The code for demo should have been put in a separate file (like the `DummyDatabase`) and not directly hard-coded in teh different activities. - It seems that you do not properly apply the singleton/factory design pattern (often the constructor is not private, etc.. See comments below). - The class hierarchy is really to understand and often we do not really see the utility, it will explained later. - Another major issue is that you do not abstract properly the authentication from the activity code. You provide a half-finished interface that do not handle login and therefore, some remaining code for Google is still in your activites code. # By file ## Package `neighborfoodandroid.adapters` ### `AllergensAdapter` - The class and the constructor should be commented (JavaDoc). ### `ConversationListAdapter` - The class and the constructor should be commented. ### `MealListAdapter` - The class and the constructor should be commented. ### `MessageListAdapter` - The class, the methods and the constructor should be commented. - Storing context is a bad practice [(see this)](https://stackoverflow.com/questions/37709918/warning-do-not-place-android-context-classes-in-static-fields-this-is-a-memory). Therefore, the context should be removed from the attributes of the class. Furthermore, it seems that the content is not used in this class... ### `OrderListAdapter` - The class and the constructor should be commented. - The initialisation of the variables `names` and `imageIds` is useless as they are initialised in the constructor. The lists `names` and `ids` should be deeply copied in the constructor. - Refer to the remark about storing context in `MessageListAdapter`. This should be avoided. Furthermore, context is not used. ### `PastOrdersListAdapter` - The class and the constructor should be commented. ## Package `neighborfoodandroid.authentication` ### `Authenticator` - The interface is half-commented. Furthermore, we are suprised not to see method such as `signIn` to be able to authenticate the user. We think that more methods could be put in this interface. ### `AuthenticatorFactory` - The class should be commented. - This class should be a singleton, i.e. constructor should be private and you should provide a `getInstance` method. - Instead of DummyAuthenticator, the basic object should be null and the getMethod should create a FirebaseAuthenticator object instead of a DummyAuthenticator as this class should only be used on tests. ### `DummyAuthenticator` - This class should be in the test folder and not in your application codebase. - The constructor should be private since you provide a `getInstance` method. And the `getInstance` method should be static. ### `FirebaseAuthenticator` - The class and the new methods should be commented. - The constructor should be private since you provide a `getInstance` method. And the `getInstance` method should be static. - The attribute `mAuth` should be create inside the `if` in the `getInstance` method. ## Package `neighborfoodandroid.database` We write the comments based on the pull request seen on April 23th. For the moment, we think that the database management is not done properly. You should rethink how this whole thing should work. We want to give you the following advices: - You should use an `Adapter` (design pattern for each of your `model` class) to transform a retrieve document from the online database into a Java Object, i.e. this object could take the JSON-like document and convert it into your Java Object using the appropriate cast. This adapter can be taken as parameters by the functions `fetch` and `fetchAll`. This way, you will be able to remove the `DocumentSnapshot` and `CollectionSnapshot` intefaces which are useless. - You will need to handle failures and success for all commands using the appropriate methods. - The new interfaces, classes and objects should be **commented**. - The method that insert data in the database could take another object that transform one of your `model` element into an object ready for the database directly in the database functions so that it is transparent for the rest of the code. - Finally, `DummyDatabase` should be in the tests. ## Package `neighborfoodandroid.models` ### `Allergen` - The `enum` is not commented. ### `Conversation` - The class is not commented. ### `Meal` - Class and methods not commented. - The second constructor could use the first one since it does the same thing, i.e. use `this(...)`. - Also, a deep copy could be made for the allergens list. ### `Message` - Class and methods not commented. - As before, can use `this(...)` for to call the first constructor instead of copy/paste the same code. ### `Model` - What is the purpose of the class ? ### `Order` - Class and methods not commented. - As before, can use `this(...)` for to call the first constructor instead of copy/paste the same code. ### `User` - Class and some methods not commented. ### `UserFirebaseImpl` - Class and some methods not commented. - Why does the empty constructor exist ? - Why is there a default value `Flen Fouleni` for the user name. It should be the empty string if the user doesn't have a `displayName` ## Package `neighborfoodandroid.repositories` ### `AuthRepository` - Wait for it..... ...... . ... Class and methods not commented. ### `AuthRepositoryFirebaseImplementation` - Class and methods not commented. - Line 8-9 why do you pass an authenticator with the `super` method and then reset it ? ## Package `neighborfoodandroid.ui` ## Package `neighborfoodandroid.ui.activities` As a general comment for the UI. You should put all the strings that are display into `string.xml` files. Furthermore, for the actvitities using the supportActionBar and redefining the onOptionsItemSelected method, you should create an abstract class that do this and make the activities inherit from this class. This way, you will avoid copy/paste of code. ### `ChatRoomActivity` - Class not commented. - `DummyDatabase.getInstance()` should be replaced by the `DatabaseFactory.getInstance()`. ### `ConversationsActivity` - Class not commented. - `DummyDatabase.getInstance()` should be replaced by the `DatabaseFactory.getInstance()`. ### `DisplayMessageActivity` - Class not commented. - This activity seems to be not really useful. Maybe you can replace it with a `Toast` or an `AlertDialog`. ### `MainActivity` - Class not commented. - The utility of the return value of the `switchFragment` method seems not be important. ### `MealActvity` - Class not commented. - (line 43) The `getStringExtra` function parameter should be a constantin the calling class or a string resource, not some hard coded string. - Moreover, we advise you to only give the id of the meal as extra to the intent and then, in the activity, write code to retrieve the data from the database for this particular meal or use `getSerializableExtra`. - (subjective) the default value for the image drawable could be something more neutral than a paella. However, the user should be able to upload its own photo of the meal, so the display should not be done as a function of the id but as a function of the image you will provide. ### `PastOrderDetailsActivity` - Class not commented. - The `getSerializableExtra` function parameter should be a constantin the calling class or a string resource, not some hard coded string. ### `PastOrdersActivity` - Class not commented - The `putExtra` function parameter should be a constant in the calling class or a string resource, not some hard coded string. - You should use the `Database` interface, even if is the `DummyDatabase`. ### `PlaceMealActivity` - Class not commented. - You should use the `Database` interface, even if is the `DummyDatabase`. - Why do we put extras in the intent if oyu don't use them. Instead of extra, you should add the meal in teh database in this class and if everything went fine, you can redirect to the MainActivity otherwise, you display an error message. - `startActivityForResult` is **deprecated**, you should not use it. ### `ProfileEditingActivity` - Class and methods not commented. - Text for the toast, should be in the string.xml file. line 150, this should also put in the string.xml file. - This class is not easily understandable, that's where comments are useful. ### `SignUpActivity` - Class and not all the methods are commented. - The whole point of making an `Authenticator` interface is to avoid using `Firebase` functions directly in the code. Therefore, all the code for the signIn should be put in the interface implementation for Firebase and not directly in the code. This links to the remark where we said that we find strange not to have a `signIn` method in the `Authenticator` interface. This should absolutely be moved to the interface. - The `startActivityForResult` is **deprecated**, you should not use it. - The texts displayed should be placed in the string.xml file. ### `VendorProfileActivity` - Class and not all the methods are commented. - Attributes should be private. - Storing the imageview objects is useless. - You should have a handling for the database since you plan on subscribing to a user. - The default url for social networks (e.g. https://twitter.com) should be stored in string resource. - The try catch in this class is handled in the same amount of time your wrote the TODO... ## Package `neighborfoodandroid.ui.fragments` ### `AccountFragment` - Class not commented. - `DummyDatabase.getInstance()` should be replaced by the `DatabaseFactory.getInstance()`. ### `ConversationsFragment` - Class not commented. - `DummyDatabase.getInstance()` should be replaced by the `DatabaseFactory.getInstance()`. - The `putExtra` function parameter should be a constant in the calling class or a string resource, not some hard coded string. ### `MealListFragment` - Class not commented. - You should use the `Database` interface, even if is the `DummyDatabase`. - The `putExtra` function parameter should be a constant in the calling class or a string resource, not some hard coded string. ### `VendorDashboardFragment` - Class not commented. - You should use the `Database` interface, even if is the `DummyDatabase`. - Useless function `initRecyclerView` ## Package `neighborfoodandroid.ui.viewmodels` ### `EditProfileViewModel` - Class and methods not commented. - Throwing runtime exception during execution is not a good practice since the expression is not catched at the other hand. ### `SignUpViewModel` - Class and methods not commented. - This class should be generic and not depend on `Firebase`. See the explaantion on `SignUpActivity` for more information. - What is the point of the `System.out.println()` on line 41 ? ## Package `neighborfoodandroid.ui.viewmodels.factories` Concretely, we do not see what is the goal of these two classes, what they represent and why there are needed in addition to `SignUpViewModel` and `EditProfileViewModel`. Maybe you can explain this to us in the comments of the classes. ## Package `neighborfoodandroid.ui` ### `AppContainer`. `AppContainerImplementation` and `NeighborFoodApplication` - Two of the three classes are useless. You should have all the value in only one class. - An `AuthRepository` and `Database` should be contained in this class. - Comments are missing - This should be a singleton. # Tests - Use mockito for unit testing instead of create a full implementation. Instead of that, we just go through the tests without really trying to understand them. But note that for the final grade, we will go over them and if we don't understand them, you'll probably be penalized. # Conclusion We hope that this code review will help you understand what you have to improve to align with the course expectation. Do not hestitate to send us an email if you have anything to ask. Cheers, Alessio and Robin.

Syntax	Example	Reference
# Header	Header	基本排版
- Unordered List	Unordered List
1. Ordered List	Ordered List
- [ ] Todo List	Todo List
> Blockquote	Blockquote
Bold font	Bold font
Italics font	Italics font
~~Strikethrough~~	~~Strikethrough~~
19^th^	19^th
H~2~O	H₂O
++Inserted text++	Inserted text
==Marked text==	Marked text
[link text](https:// "title")	Link
![image alt](https:// "title")	Image
`Code`	`Code`	在筆記中貼入程式碼
```javascript var i = 0; ```	`var i = 0;`
:smile:		Emoji list
{%youtube youtube_id %}	Externals
$L^aT_eX$	L^aT_eX
:::info This is a alert area. :::	This is a alert area.