----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46504/#review131919 -----------------------------------------------------------
3rdparty/libprocess/include/process/http.hpp (lines 584 - 603) <https://reviews.apache.org/r/46504/#comment195961> Is this a strict format? That is, are we required to phraes it this way? Can we just take `allowMethods` by value, and do the following? ``` foreach (std::string& method, allowMethods) { method = "'" + method + "'"; } return "Expecting one of: " + strings::join(", ", allowMethods) + ", but received '" + requestMethod + "' "; ``` 3rdparty/libprocess/include/process/http.hpp (lines 616 - 617) <https://reviews.apache.org/r/46504/#comment195960> Why not just use a lambda? ```cpp MethodNotAllowed( const std::initializer_list<std::string>& allowMethods, const std::string& requestMethod) : Response( [&allowMethods, &requestMethod]() { std::string body = "Expecting "; if (allowedMethods.size() == 1) { body += "'" + allowedMethods.begin()[0] + "'"; } else if (allowedMethods.size() == 2) { body += "'" + allowedMethods.begin()[0] + " or " + "'" + allowedMethods.begin()[1] + "'"; } else { std::initializer_list<std::string>::iterator it; std::size_t ind; for (it = allowedMethods.begin(); it != allowedMethods.end(); ++it) { ind = it - allowedMethods.begin(); if (ind == allowedMethods.size() - 1) { body += "or '" + *it + "'"; } else { body += "'" + *it + "', "; } } } body += ", received '" + requestMethod + "'"; return body; }(), Status::METHOD_NOT_ALLOWED) {} ``` If you want to define it out-of-line, why not just a private static function? ```cpp MethodNotAllowed( const std::initializer_list<std::string>& allowMethods, const std::string& requestMethod) : Response( createBody(allowMethods, requestMethod), Status::METHOD_NOT_ALLOWED) {} private: static std::string createBody( const std::initializer_list<std::string>& allowMethods, const std::string& requestMethod) { std::string body = "Expecting "; if (allowedMethods.size() == 1) { body += "'" + allowedMethods.begin()[0] + "'"; } else if (allowedMethods.size() == 2) { body += "'" + allowedMethods.begin()[0] + " or " + "'" + allowedMethods.begin()[1] + "'"; } else { std::initializer_list<std::string>::iterator it; std::size_t ind; for (it = allowedMethods.begin(); it != allowedMethods.end(); ++it) { ind = it - allowedMethods.begin(); if (ind == allowedMethods.size() - 1) { body += "or '" + *it + "'"; } else { body += "'" + *it + "', "; } } } body += ", received '" + requestMethod + "'"; return body; } ``` In either case, we wouldn't need the `MethodNotAllowedBody` class nor the `MethodNotAllowed(MethodNotAllowedBody member);` constructor. - Michael Park On May 3, 2016, 12:20 a.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46504/ > ----------------------------------------------------------- > > (Updated May 3, 2016, 12:20 a.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-4126 > https://issues.apache.org/jira/browse/MESOS-4126 > > > Repository: mesos > > > Description > ------- > > Constructed error string in MethodNotAllowed. > > - Decided to use constructor delegation rather than just constructing in > MethodNotAllowed > allowing body to be constructed before calling the Response constructor. > This in addition > to the improved readability and sectioning off the verbose body > construction as a middle step > in a delegation chain led me to this implementation. Noting here that > construction in > MethodNotAllowed would be completely valid, but seeing what others think of > this choice. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/http.hpp > 8f4eabcbb71ead1f5c28e1d8a2dd40db7af1f297 > > Diff: https://reviews.apache.org/r/46504/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jacob Janco > >
