This differential refactors all getAll.... -like queries not to use ORM.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
When I initially scoped this task I wanted to make it more maintainable, to somehow parse the SQLite result based on the struct definition, or at least somehow enrich entities to make it more readable than using some magic numbers.
When looking at this right now this seems complicated, you will end up implementing our own orm code which is or uses some advanced mechanism like reflection so it's probably okay as it is, given that you already implemented all functionalities.
Overall idea LGTM, couple of inline comments related to code quality.
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 1119–1121 ↗ | (On Diff #35794) | we can line this I think (if you agree please update entire diff) |
| 1135–1136 ↗ | (On Diff #35794) | I think it's a good time to unify SQL formatting in C++ code which is not good currently and match the style we have on a keyserver. What do you think? |
| native/cpp/CommonCpp/DatabaseManagers/entities/Draft.h | ||
| 14 ↗ | (On Diff #35794) | How about using a safer static_cast? |
| native/cpp/CommonCpp/DatabaseManagers/entities/EntityQueryHelpers.h | ||
| 1 ↗ | (On Diff #35794) | we should add #pragma once in each header file |
| 10–37 ↗ | (On Diff #35794) | I think we can wrap this in a class, put finalizePreparedStatement in destruct, etc. After looking at the code later in the stack this might simplify things a bit |
| 46–47 ↗ | (On Diff #35794) | I think this is more efficient |
| native/cpp/CommonCpp/DatabaseManagers/entities/Message.h | ||
| 19–42 ↗ | (On Diff #35794) | I would implement some utils functions like createString, createStringPtr createInt, etc, and pass row and index, and inside those functions implement casting, and create pointers if needed. Then creating structure will be more readable and we can easily add some error handling. We can also easily reuse it later. What do you think? |
| native/ios/Comm.xcodeproj/project.pbxproj | ||
| 481 ↗ | (On Diff #35794) | I think you can also add this in native/cpp/CommonCpp/DatabaseManagers/CMakeLists.txt |
Comment Actions
- Introduce convention to SQL on native.
- Wrap sqlite prepared statement object into high-level C++ class.
- Move data conversion from sql result into separate file
| native/cpp/CommonCpp/DatabaseManagers/entities/Draft.h | ||
|---|---|---|
| 14 ↗ | (On Diff #35794) | Unfortunately casting from const unsigned char* to const char* cannot be performed with neither static_cast nor const_cast. reinterpret_cast is the only option here. |
Comment Actions
Looks good but please fix the rebasing issue with @""generated or if this is intended - create a separate diff for that
| native/cpp/CommonCpp/DatabaseManagers/entities/SQLiteDataConverters.h | ||
|---|---|---|
| 1 ↗ | (On Diff #36376) | not sure if /entities is the best directory for these files - but it's probably okay |
| web/scripts/run_emscripten.sh | ||
| 165 ↗ | (On Diff #36376) | is it some kind of rebase issue? |
| web/scripts/run_emscripten.sh | ||
|---|---|---|
| 165 ↗ | (On Diff #36376) |
I don't understand this question.
Do you think there is some alternative solution that would keep run_emscripten.sh visible without showing comm-query-executor.js? If not then I am inclined to revert this change as I think showing comm-query-executor.js is worse than not showing run_emscripten. |
| native/cpp/CommonCpp/DatabaseManagers/entities/SQLiteDataConverters.h | ||
|---|---|---|
| 1 ↗ | (On Diff #36376) | I decided to put this file to entities directory since it is essential for entity <-> SQL interop. |