Defines in C++ functions that are implemented in message-ops-utils.js
Details
No testing plan. Future diffs will introduce code that was used on iOS and Andrdoid to parse raw message strings and store them in database.
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
This diff is quite big, it might be a good idea to add each function in a separate diff.
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp | ||
---|---|---|
17 ↗ | (On Diff #12415) | why do you use snake instead of camel case? |
32 ↗ | (On Diff #12415) | How does this work with folly::dynamic? Don't we need to check whether it is a collection and if the field is present in the first place? |
44–47 ↗ | (On Diff #12415) | camel case |
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.h | ||
26 ↗ | (On Diff #12415) | new line missing |
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp | ||
---|---|---|
32 ↗ | (On Diff #12415) | We can always iterate in foreach manner over folly::dynamic provided it is an Object or an Array. In the first case the iteration is similar to iteration over dictionary (key-value) pairs and similar to iteration over vector in the latter. I understand there are several places in this code where I assume that a certain key is present or the value under certain key is an Object or an Array. On one hand it would be justifiable to add sanity checks in those places and throw some error/log and return if the condition is not fulfilled. On the other this code is meant to mirror functionality implemented in message-ops-utils.js where assumptions about a presence of a certain key or character ot the value are not additionally verified. Therefore I assumed it might be an overkill to introduce such sanity checks in C++ version of the code. However it is not an issue to provide such checks. @ashoat, @palys-swm what do you think? Can I assume that if presence of a key or value character in notification is not verified in JS then it does not need to be verified in C++? |
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp | ||
---|---|---|
32 ↗ | (On Diff #12415) |
In our JS codebase, I think these assumptions are generally enforced statically by the Flow typechecker. But I don't think data returned from the server is verified at runtime, so the assumptions made by the typechecker could be wrong. I think it's probably okay to make these assumptions since we do the same in JS, but I guess it would be better to do some sort of runtime checks when the data is received from the server. I guess that's probably outside the scope of this diff? I honestly don't feel too strongly, open to other perspectives. |
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp | ||
---|---|---|
32 ↗ | (On Diff #12415) | I wouldn't say it is out of the scope of this diff. Instead of optimistically accessing keys we could use if statements or ternary operators to check for presence of given key or whether value we expect under the given key is indeed and Object or an Array so that it can be iterated over. The only difficulty I see in performing runtime checks is how should we react when those checks fail? Probably we should throw an error or log error message and return, but how do we make sure that corrupted notification is re-send in a correct form, or is at least downloaded in its correct form upon app launch? |
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp | ||
---|---|---|
32 ↗ | (On Diff #12415) | It may be good to create a helper function for this. It would assign a value if it is present or throw/do sth else when it is not present. |
I think it's fine to don't have a test plan for a diff which introduces just the interface. We could also mention that e.g. it compiles.
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.h | ||
---|---|---|
12 ↗ | (On Diff #12477) | Why do we have a class with all the functions static? What is the benefit of this approach compared to plain functions in comm namespace? |
14 ↗ | (On Diff #12477) | It might be a good idea to have e.g. a typedef for it - it would be more readable to see what std::pair<Message, std::vector<Media>> actually means for us |
22 ↗ | (On Diff #12477) | This function returns a vector, so maybe we should use plural messageInfos in the name? |
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.h | ||
---|---|---|
12 ↗ | (On Diff #12477) | I am about to make some of those functions private since they may throw exception that will be handled in public methods of this class. Another reason to have this functionality contained within a class is that we are going to use them in Java on Android in CommNotificationsHandler.java |
Use typedef for pair of Message and vector of Media. Use private access specifier for functions that may crash.
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.h | ||
---|---|---|
14 ↗ | (On Diff #12550) | All the members of a class are private by default, so there's no need to specify that |
24 ↗ | (On Diff #12550) | THis code doesn't seem to be properly formatted. We probably need to update clangPaths |
24–25 ↗ | (On Diff #12550) | As suggested in other diff, should we take these by reference? |
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.h | ||
---|---|---|
24 ↗ | (On Diff #12550) | Do we know if this is properly formatted? |
native/android/app/CMakeLists.txt | ||
---|---|---|
75–76 ↗ | (On Diff #12725) | Would be nice if there was a way to just tell CMake to recursively include a directory and all of its subdirectories. @karol-bisztyga found something like this, but not for include_directories I think. @jimpo, any ideas? |
native/android/app/CMakeLists.txt | ||
---|---|---|
75–76 ↗ | (On Diff #12725) | Ping on this @marcinwasowicz @karol-bisztyga @geekbrother @jimpo any way to recursively include a directory in CMake? |
native/android/app/CMakeLists.txt | ||
---|---|---|
75–76 ↗ | (On Diff #12725) | Something like file(GLOB_RECURSE SOURCE_CODE "./src/*.cpp") works for finding files but here we need to find directories here. |