> On May 5, 2016, 8:49 p.m., Michael Park wrote: > > 3rdparty/libprocess/include/process/http.hpp, lines 616-617 > > <https://reviews.apache.org/r/46504/diff/2/?file=1369337#file1369337line616> > > > > 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.
Having lambda is too cryptic, but since we got rid of the state in `MethodNotAllowed` we can easily replace it with a static function. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46504/#review131919 ----------------------------------------------------------- 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 > >