plugin: add new external class for user/bank information #403
Conversation
cmoussa1
added
improvement
|
@cmoussa1, sorry I keep trying to go through this PR and getting interrupted. I think this is a good start, and definitely seems to clean up some calls in the plugin. However, what I was picturing would go a bit further and make a real However, I'm afraid I'd have to study the code a bit more and get familiar with the interfaces such a class would need in order to make any useful suggestions. Also, I'm a novice at C++ at best, and perhaps wouldn't be the best person to make suggestions or review C++ code. But, I'll try to go through the plugin again and maybe make some suggestions along the lines of what I was thinking. |
|
Here's a couple examples after a quick look:
Maybe in a comment you could summarize the data structures that are currently being used by the plugin, and the functions they need, and we can then work on the design of an abstraction that could encapsulate all this within one or more classes? O/w, perhaps we could walk through the code at some point (maybe on a coffee call) and kind of wrap our heads around it that way? |
|
@grondo no problem at all, thanks for beginning to take a look at this last week! Just got back from vacation so am just getting back to this now. I'm sorry I probably didn't make it very clear in the PR description, but the intent with this PR at least was to just begin moving some of the definitions/methods of the In any case, I completely agree with you that perhaps a separate comment/issue outlining what methods should be moved out of the plugin and into a separate class should be opened. I can work on it this week and open it up for further discussion where folks who are more familiar with C++ than I am can also chime in with their expertise! |
|
Sounds great! |
Problem: the bank_info struct does not contain the name of the bank in the struct, which can make it more tedious to find the name of the bank because you have to look at the key of the map in order to find it. It would be more convenient if the name was also in the struct. Add the bank name to the struct attached with each user/bank combination.
Problem: The plugin uses its own bank_info struct to hold user/bank information and has a number of methods for access and modification of these structs. As the plugin's feature set has grown, so have the requirements for the bank_info struct, resulting in a very large and hard-to-parse piece of code. Begin to clean up this plugin. Start by creating a new user_bank_info class and place it in a separate file that gets compiled with the plugin. Replace all instances of "struct bank_info" with the new "user_bank_info" class type.
Problem: There is some functionality in the priority plugin (mf_priority.cpp) that is specific to accessing user_bank_info objects in a map data structure. These methods can be abstracted out to the class files to clean up some of the plugin code. Add a new function to the bank_info.cpp class file that handles looking up a user/bank combo and fetching a user_bank_info object from the users map, or returning NULL on failure.
Problem: There are no unit tests for the helper functions in bank_info.cpp. Begin to add some unit tests for these functions.
Problem: The arguments to the get_queue_info () function include an iterator to the bank_info object, which is used in the function to access the user/bank's list of permissible queues for validation. This can be confusing to read and would be easier if just the list of permissible queues was passed to the function. Change the argument to the get_queue_info () function from an iterator to the user/bank object to just the user/bank's list of permissible queues. Change the calls to this function to account for the argument change.
Problem: There are now helper functions available for the plugin to make use of to help with lookups and access of the internal map for users and banks, but the plugin does not make use of them. Restructure the validate_cb () function to make use of the new get_user_info () helper function. Change the error message for when a user/bank cannot be found and adjust the respective tests that look for this error message. Improve the comments in this function/add new ones to help with some of the clarity in this function.
Problem: There are a couple variables in validate_cb () that are unused. Remove them.
cmoussa1
force-pushed
the
abstract.plugin.features.1
branch
from
52a8f7a to
edd1082
Compare
Problem: The helper function check_map_for_dne_only () is specific to the internal maps that store user/bank information in the priority plugin. It might make more sense to include this in the external files that are centered around user/bank information instead. Move check_map_for_dne_only () to bank_info.cpp. Add both maps that store user/bank information as arguments to the function. Edit the calls to this function in validate_cb () and priority_cb () to include both maps as arguments.
Problem: There are no unit tests for the helper function check_map_for_dne_only (). Add some basic unit tests.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #403 +/- ##
==========================================
+ Coverage 81.00% 81.86% +0.86%
==========================================
Files 20 23 +3
Lines 1437 1500 +63
==========================================
+ Hits 1164 1228 +64
+ Misses 273 272 -1
|
|
note: I've added a couple commits to this PR that move the I've continued working on walking through the plugin and moving things out as I notice room for improvement, but for the most part I've kept them in separate branches (to be proposed as separate, future PRs) to prevent overloading this one into a huge PR that's a pain to manage & review. Progress is being tracked in #403 on what work is located where at the moment 😅 |
|
FWIW, I think it could be helpful take a step back and describe the potential classes used in the mf_priority plugin and their methods. For instance, it feels like the two functions added here should be methods of a class and not just functions. Then once you have a design for the classes and methods, you could implement them with unit tests. This would be very easy to review. Finally, the plugin could be updated (even piecemeal) to use the new classes. This would also be pretty easy to review, and would be mostly vetted by the testsuite still passing. This is how I would approach the problem anyway. Unfortunately, I don't have enough insight into the current plugin to offer much more advice. |
|
Also, it might be useful to pair up on the design phase here. Maybe you could walk through the current design with someone and make a first pass on the design. I don't think I'm the best person for this since I'm not very adept at C++, but perhaps someone else on the team might have the expertise to help (e.g. @jameshcorbett?) Edit: this is just my suggestion! If you feel you've got a good approach going, then feel free to ignore me 🙂 |
|
Oh, that's a great suggestion @grondo! I think having another set of eyes during design would be super helpful. Yes, I should take a pass on this approach and see where we get. We can propose stuff in an issue/discussion thread or a coffee call as you've mentioned. :-) |
I'm down! |
|
This work is going to be re-proposed in a more concise manner (such as in #411), so I'll close this PR. |
Background
The plugin uses its own
bank_infostruct to hold user/bank information and has a number of methods for access and modification of these structs. As the plugin's feature set has grown, so have the requirements for thebank_infostruct, resulting in a very large and hard-to-parse piece of code. It would benefit the readability of this plugin if some basic features were abstracted out to an external class that the plugin could just#include.This PR begins this process of moving some of the definitions and features related to accessing user/bank information from the plugin's internal map to an external class.
bank_info.hppandbank_info.cpp, in this PR, is proposed to now hold the following:get_user_info (), which will perform a lookup of a user/bank in the internal map and return its location in the map orNULLon failureReferences to this new user/bank class (called
user_bank_info) are now used in place of the oldstruct bank_info.I was also able to add some basic unit tests for this
get_user_info ()helper function that get executed withmake check. This test suite can grow as more and more features are abstracted out of the plugin. :-)As a result of this new class and helper function, I also adjusted one of the other helper functions that validates a queue for a user/bank by changing one of its function arguments to just the user/bank's list of permissible queues (instead of the map iterator to the old
bank_infoobject, which was ugly to look at).Finally, in the last commit, I give an example of what using these new external
.hppand.cppfiles does to a function in the plugin in terms of cleanup and improved readability.validate_cb ()is adjusted to use this new helper function for the user/bank lookup and no longer needs to define and use a map iterator anywhere in its function. I've also added/adjusted some of the outdated comments in this function to help clarify some of the checks in this callback with the intention to help explain what it is doing more clearly.For the purposes of (hopefully) keeping this PR concise, I've just adjusted
validate_cb ()to showcase this new use of the external.cppfiles; in the future, I can open similar, subsequent ones to improve each callback.