Rename some variable so that all this-> can be dropped, and do so.
#180
Conversation
… rm_this Rebase
bcsgh
changed the title
this-> can be dropped, and do so. etr#179this-> can be dropped, and do so.
Codecov Report
@@ Coverage Diff @@
## master #180 +/- ##
=======================================
Coverage 95.19% 95.19%
=======================================
Files 35 35
Lines 3140 3140
=======================================
Hits 2989 2989
Misses 151 151
Continue to review full report at Codecov.
|
|
I think the failing coverage is another case of pre-existing conditions. |
etr
left a comment
etr
left a comment
There was a problem hiding this comment.
Thanks a lot for removing redundancy. The only ones I'd prefer to leave the same are the explicit setters where I believe "this" gives us a more explicit sense of the fact that a property of the object is being changed. The rest looks good.
src/http_request.cpp
Outdated
| }; | ||
|
|
||
| void http_request::set_method(const std::string& method) | ||
| void http_request::set_method(const std::string& method_src) |
There was a problem hiding this comment.
I'd leave this specific one as is given that it is a setter method
src/httpserver/http_request.hpp
Outdated
| void set_content(const std::string& content_src) | ||
| { | ||
| this->content = content.substr(0,content_size_limit); | ||
| content = content_src.substr(0,content_size_limit); |
There was a problem hiding this comment.
as for the other setter I commented, I'd leave this one with "this" that I find more expressive in this case
src/httpserver/http_request.hpp
Outdated
| void set_content_size_limit(size_t content_size_limit_src) | ||
| { | ||
| this->content_size_limit = content_size_limit; | ||
| content_size_limit = content_size_limit_src; |
src/httpserver/http_request.hpp
Outdated
| void set_path(const std::string& path_src) | ||
| { | ||
| this->path = path; | ||
| path = path_src; |
src/httpserver/http_request.hpp
Outdated
| void set_version(const std::string& version_src) | ||
| { | ||
| this->version = version; | ||
| version = version_src; |
src/httpserver/http_request.hpp
Outdated
| * @param args The args key-value map to set for the request. | ||
| **/ | ||
| void set_args(const std::map<std::string, std::string>& args) | ||
| void set_args(const std::map<std::string, std::string>& args_src) |
test/integ/basic.cpp
Outdated
| print_request_resource(std::stringstream* ss_src) | ||
| { | ||
| this->ss = ss; | ||
| ss = ss_src; |
test/integ/basic.cpp
Outdated
| print_response_resource(std::stringstream* ss_src) | ||
| { | ||
| this->ss = ss; | ||
| ss = ss_src; |
|
Regarding the setters; I fully agree with your intent, but I strongly disagree with your proposed means. The intent of making it clear where a variable is coming from the object vs. somewhere else is rather desirable, but not just in setters; it is my opinion that that should be done universally. As noted in the PR description, I'd suggest accomplishing that intent (universally) by making the form of member variable names distinct from locals. The If that is going to be the done, then making that transformation as a follow-on PR would be the preferred next step[2] and dropping these All that said, that's my opinion, and this is your code. So I'll just make my case and let you decide. [1]: I personally prefer trailing |
|
I appreciate the feedback but I'd stay with setters parameters matching the object internal one and "this" used. The general rule of thumb is that "this" is /almost/ always redundant. The setters are where /almost/ has value for me in that sentence. I agree with the rest of your CR though, as elsewhere, it is redundant. create_webserver has been managed differently to allow for members to be named to match those in the webserver class. On the protected members, sadly that has been poisoning this repo for a long time. Admittedly, they need better handling that I didn't have the time to do. Some of these things have been in the repo for quite a while as the project itself has been around for ~10 and has accumulated tech debt. |
|
Additional note that I think wasn't clear at all from my previous comment. I would be totally happy with a subsequent change to enforce the use of "_" in front of all class members (thus making "this" redundant for setters). In the context of this PR, I rather prefer having setters using "this" than having parameters with different names. |
|
Everything merged. Thanks again for your support to the library. |
3 participants
Identify the Bug
#177 (General code style issues)
Description of the Change
Rename some variable so that all
this->can be dropped, and do so.this->is almost always redundant.Side Note: the cases where this required changing the name of local variable to not conflict with member variable is the reason that many style guides (e.g. https://google.github.io/styleguide/cppguide.html#Variable_Names) require different forms for those types of variables. It may be worth renaming all private member variables at some point.
Alternate Designs
n/a
Possible Drawbacks
none really.
Verification Process
Normal build process. CI build to follow
Release Notes
this->.