Skip to content

Code improvement#9

Merged
MMquant merged 17 commits intoMMquant:developfrom
blasvicco:Code-Improvement
Oct 23, 2018
Merged

Code improvement#9
MMquant merged 17 commits intoMMquant:developfrom
blasvicco:Code-Improvement

Conversation

@blasvicco
Copy link
Copy Markdown

@blasvicco blasvicco commented Sep 30, 2018

Change log

  • 2018-09-30 Adding test file.
  • 2018-09-30 Adding Request object.
  • 2018-09-26 Using the small Docker image Alpine instead of Debian.
  • 2018-09-26 Using docker-compose to build/up/down the image.
  • 2018-09-26 Grouping project files inside the app folder.

Motivation

Creating the first test file.
Reducing code lines from BitfinexAPI.hpp and cleaning it moving the curl handlers to the Request class.
Using a small docker image.
Using docker-compose to sync the app folder and its content between the docker image and the host machine.
Avoiding clone the project inside the docker image.
Separating the project file from the docker files.

TODO

Test the post action.

@blasvicco blasvicco changed the base branch from master to develop September 30, 2018 19:03
Copy link
Copy Markdown
Owner

@MMquant MMquant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

#include "error.hpp"

// internal TRequest
#include "TRequest.hpp"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though I'm clever but now I see I'm not. I can't figure out what means T in TRequest.hpp :). I would rename it to "two word name" T....Request.hpp.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for it. It is just my way to define class, so then we can instance with the name of the object. So in this case it is TRequest so then we can do:

TRequest Request = new TRequest();
Request.get(...);

So in my mind T is Template like a way to call the recipe of abstraction (class) of the object (instance). But of course we can name it as you prefer.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking in TRequests.hpp and I would rename all occurrences of TRequest to HTTPRequest which will improve code clarity.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MMquant sorry for the delay. I will change as soon as I have a moment.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change to HTTPRequest even if the request is not limited to the HTTP protocol.

////////////////////////////////////////////////////////////////////////
// Private properties
////////////////////////////////////////////////////////////////////////
string endpoint = "";
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think string initialization string endpoint = ""; is unnecessary. Simple less verbose string endpoint; should do the same.

(1) empty string constructor (default constructor)
Constructs an empty string, with a length of zero characters.

void setupHeader() {
curlHeader = nullptr;
map<string, string>::iterator it;
for (it = header.begin(); it != header.end(); it++) {
Copy link
Copy Markdown
Owner

@MMquant MMquant Sep 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this code be simplified such ...
delete map<string, string>::iterator it;
change for (auto it = header.begin(); it != header.end(); ++it) {
?
But it's up to you if you prefer more verbose way. auto keyword comes handy in these cases.

@MMquant MMquant merged commit fc03e1b into MMquant:develop Oct 23, 2018
MMquant added a commit that referenced this pull request Nov 9, 2018
* Removing not needed files.

* Moving project source into app folder.

* Adding docker files for alpine distro.

* Updating README.md.

* Updating git ignore.

* Code improvement (#9)

* Removing not needed files.

* Moving project source into app folder.

* Adding docker files for alpine distro.

* Adding test.

* Moving all the curl calls and settings to the Request object.

* Formatting header.

* Making key-secret file optional.

* circleci fix
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.

2 participants