Skip to content

Event Refactor Phase 1#427

Closed
jmjatlanta wants to merge 6 commits intoGLEECBTC:research-archivefrom
jmjatlanta:jmj_event_tests
Closed

Event Refactor Phase 1#427
jmjatlanta wants to merge 6 commits intoGLEECBTC:research-archivefrom
jmjatlanta:jmj_event_tests

Conversation

@jmjatlanta
Copy link
Copy Markdown

@jmjatlanta jmjatlanta commented May 10, 2021

I would like to use this PR to open a dialog into integrating heavier use of C++ features into the codebase. Comments are encouraged. But please send any comments about what I should or should not be working on directly to me instead of commenting here.

Status as of 2021-05-10: I believe the code sufficiently demonstrates concepts that could be used in further refactoring. I believe the code to be bug-free, but not complete, as both old and new ideologies coexist for discussion and testing purposes. More test cases are needed for edge cases that I have as yet not learned about.

Points for discussion:

  1. Implementations in header files: Much of the code I have seen contains little implementation in .cpp files and most in .h header files. The disadvantages of this approach are (1) increased compile times and (2) delicate dependencies in linking libraries or executables. Moving the implementations to .cpp files does have disadvantages as well, such as objects compiled with different parameters can cause different (sometimes hard to diagnose) errors, but it is my opinion that the benefits outweigh the dangers.
  2. Use the standard library where it makes sense to avoid platform-specific code. Two changes highlight the benefits here. Mutexes (std::mutex insted of pthread's version), and lists (std::list vs ptr-to-ptr lists).
  3. Remove use of pointers where possible (i.e. pass references if possible, const references are even better), use reference counted pointers instead of raw pointers. The std::list vs ptr-to-ptr is an example here as well.
  4. Declare necessary variables when you need them, not at the beginning of the function. This often makes the code easier to read.
  5. Use exceptions where it makes sense. If the code is littered with if statements for error checking, consider using try/catch. This can reduce the code you need to write, as well as provide a performance boost in some instances if done correctly.
  6. Use namespaces. Instead of starting each method name with komdo_, consider using namespace komodo. This reduces name collisions within the codebase (we know what an "event" is in komdo, and don't have to worry about puting "komodo_" in front of it, or a UI event having a name collision with it). This also often makes using your code as a library less cumbersome.
  7. Use bool if you only need true/false instead of other integrals. This makes your intent clear.
  8. Comment things that may be unclear. I personally use doxygen style comments on top of my functions. I hope everyone can understand my code by reading it, but sometimes it is very inconvenient to do so. Of course the evil side of comments is they are often stale or inaccurate. But the intent often remains.
  9. Push implementations to where they belong. A "notary event" needs to be serialized. It is probably best that the code to do so reside close to the definition of what a notary event is. Every serialized "event" has a header, so this is probably a good time to use a hierarchy of "notary_event is an event, and knows how to serialize itself". Now reading or writing it to disk only requires code to tell a notary event to serialize/deserialize itself. The implementation code stays centralized and out of the way.
  10. Refactor a little at a time. Knowing then what is known now, everyone would love to rewrite pretty much everything we've ever written. Avoid the urge. Bite-sized subsystems can be tackled. Eating the whole elephant will take time. The majority of the komodo_events.h file breaks point 9 above. I'd love to get rid of it, as well as big chunks of komodo.h for the same reason. But that would make testing and comparing the existing implementation with the refactor too much to take on in this round. So they will remain until they are no longer needed.
  11. Header files should be independent. Include all the #include files necessary for the .h. This makes chasing down dependencies less of a problem at the expense of a little compile time. Use #pragma once to help with this.

The above points are simply opinions. I am not here to start wars. If the team decides that any (or all) of these are a bad idea, I'll happily work without using them. Explaining why they are a bad idea helps everyone, but is not required.

Reading suggestions and References:

@ca333 ca333 requested review from Alrighttt and DeckerSU May 10, 2021 15:36
@dimxy
Copy link
Copy Markdown
Collaborator

dimxy commented May 10, 2021

I basically support all of that.
We should be careful when refactoring consensus code.
I think yet we could move from using boost to the basic c++ constructs, within c++11 standard

@ca333
Copy link
Copy Markdown

ca333 commented May 10, 2021

thanks for feedback and proposals - agree on all 11 points.

@dimxy
Copy link
Copy Markdown
Collaborator

dimxy commented May 11, 2021

good to add yet some policy on git commits (like make each commit for a single purpose, add descriptive messages, do not make commits that can't compile)

@ca333 ca333 requested a review from dimxy May 11, 2021 17:50
@Milerius
Copy link
Copy Markdown

good to add yet some policy on git commits (like make each commit for a single purpose, add descriptive messages, do not make commits that can't compile)

I use that in atomicdex desktop https://www.conventionalcommits.org/en/v1.0.0/ for conventional to follow, they have an external tool to generate changelog based on this commit convention, which is quite cool

This was referenced May 18, 2021
@jmjatlanta
Copy link
Copy Markdown
Author

Closed in favor of #440

@jmjatlanta jmjatlanta closed this May 18, 2021
who-biz pushed a commit to who-biz/komodo that referenced this pull request Jun 14, 2022
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.

4 participants