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

Add cookbook support. #2281

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Add cookbook support. #2281

wants to merge 5 commits into from

Conversation

Leptopoda
Copy link
Member

closes: #353
Best to be reviewed on a per commit basis.
This is a first implementation and somewhat port of the cookbook app by @Teifun2 (https://github.com/Teifun2/nextcloud-cookbook-flutter). The UI is still garbage and some code like the category grid layout is just garbage (it does the job bit isn't really clean).
I'm currently bypassing the entire RequestManager as I didn't want to mess with the behavior subjects. The caching functionality will come once #2165 and #2155 are done.

I'm not really interested in a review of the code style or the UI. I'd rather like to discuss the architecture of the cookbook app (provider, repository and bloc). for better testability and mockability (I'll add some tests in the coming days to show this).

The file structure might be a bit unconventional in comparison to our current one. But the benefits are quite enormous as adding further widgets, utils and even entire new screens does not need to change any imports. Changes to a screen and it's logic will also contain the diff to that directory making reviews easier.
I've deliberately kept the pages and views separate so we can have a material design view and a cupertino one.

I'm open to discuss everything in detail.

@Leptopoda Leptopoda requested a review from provokateurin July 16, 2024 19:18
@Leptopoda
Copy link
Member Author

I still need to mention the history of the cookbook app and link to the old app in the cookbook readme.

@provokateurin
Copy link
Member

How about extracting and merging the first two commits already? Then this PR gets a bit smaller and they both make sense even standalone.

@provokateurin
Copy link
Member

provokateurin commented Jul 16, 2024

I had a first look and I have no major objections, but I feel like there is a huge amount of boilerplate code. Maybe this allows for a super clean architecture, but I feel like the trade-off isn't worth it.
Take a look at these stats:

neon_talk, with tests included
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Dart                            39            837            298           6148
YAML                             4              5              0             78
-------------------------------------------------------------------------------
SUM:                            43            842            298           6226
-------------------------------------------------------------------------------
neon_cookbook, with tests included
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Dart                            61            497            429           2840
YAML                             7              8              2            101
-------------------------------------------------------------------------------
SUM:                            68            505            431           2941
-------------------------------------------------------------------------------

neon_talk, without tests
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Dart                            25            393            286           2553
YAML                             4              5              0             78
-------------------------------------------------------------------------------
SUM:                            29            398            286           2631
-------------------------------------------------------------------------------
neon_cookbook, without tests
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Dart                            53            378            428           1854
YAML                             7              8              2            101
-------------------------------------------------------------------------------
SUM:                            60            386            430           1955
-------------------------------------------------------------------------------

While Talk and Cookbook have different challenges, we can probably agree that Talk has a lot more functionality (and complexity) at the moment while Cookbook has about 73% of LOC of Talk. This strongly indicates that the LOC to functionality ratio is a lot worse for Cookbook which just means it has a lot of boilerplate.

Let me know what you think about this issue, I'm interested to hear your arguments. As I said in the beginning I'm with this architecture as long as you think it is the right way to do it.

@Leptopoda
Copy link
Member Author

I don't think that this comparison makes any sense. The cookbook api and the way dynamite generates the dart code is quite different from what talk has to offer. The cookbook app also can't make use of common widgets from the framework as much as talk does.
Let's look at the following

------------------------------------------------------------------------------------------------------------
Language                                                  files          blank        comment           code
------------------------------------------------------------------------------------------------------------
lib/**/bloc/**/*.dart                                        6             39             39            146
lib/**/view/**/*.dart                                        9             27             15            282
lib/**/widgets/**/*.dart                                     8             50             94            298
# recipe_repository
packages/recipe_repository/lib/src/recipe_repository.dart    1             41             56            155
packages/recipe_repository/lib/src/models                    7            127             64            577
packages/recipe_repository/lib/src/utils                     5             20             30            160
  1. we shouldn't count the widget and view code. This is app specific and there's nothing one can change about that
  2. let's not count the models and utils from the repository. They'd still be needed with our current architecture
    1. dynamite can not work with dates and durations.
    2. the category definition of the server is garbage and the app needs to have an easy way to retrieve the cover picture

This only leaves ~300 loc for the business logic. Considering that the repository already contains everything needed for the recipe view (and creation, edit) this is a rather normal number.


For this discussion, we should rather focus on the architecture. There isn't a single line of business logic in the widget code.
This means changing the UI (or making it responsive/adaptive) is way easier as one only has to present the data (presentation layer) and not also change it.

Another benefit of the separate repository layer beneath the blocs is that Christian wants to eventually migrate the API to OCS to leverage everything associated with it. Even if he were to change it entirely (like a usable categories endpoint) or using a different data model the blocs wouldn't need to change.

A bit off topic

My long term dream app could manage my recipes, groceries and even the money flow between flat mates. Nextcloud can offer all of them using Notes, Cookbook and Cospend. If all of their api quirks were to be abstracted into reusable repositories, I could just import the notes and cospend ones to incorporate them into the current app.

@provokateurin
Copy link
Member

The cookbook api and the way dynamite generates the dart code is quite different from what talk has to offer

Right, the Talk API is pretty good and I don't need to convert anything into my own models.

The cookbook app also can't make use of common widgets from the framework as much as talk does.

Talk uses a lot less widgets from the framework than you think.

For this discussion, we should rather focus on the architecture. There isn't a single line of business logic in the widget code.
This means changing the UI (or making it responsive/adaptive) is way easier as one only has to present the data (presentation layer) and not also change it.

Another benefit of the separate repository layer beneath the blocs is that Christian wants to eventually migrate the API to OCS to leverage everything associated with it. Even if he were to change it entirely (like a usable categories endpoint) or using a different data model the blocs wouldn't need to change.

I get that the decoupling is nice in this case to fix the problems the API has, but this kind of effort is not necessary in most places. Adding more layers in between might make for a nice architecture and allows easy changes, but it automatically means there is a lot more code to maintain and it is much harder for anyone to get into this architecture than what we currently have.

My feeling is basically that this adds a lot of complexity to achieve a nice architecture, while something much simpler would do the job just as good and make it easier to develop on it. So for me it's a trade-off that doesn't make sense, but if you are fine with it then go for it.

My long term dream app could manage my recipes, groceries and even the money flow between flat mates.

I love that idea :D

@Leptopoda Leptopoda force-pushed the feat/neon/cookbook branch from 8854e36 to 44f0712 Compare July 21, 2024 22:11
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

Successfully merging this pull request may close these issues.

Implement Cookbook App
2 participants