Замечания по CHttpURL
Closed this issue · 16 comments
alexey-malov commented
alexey-malov commented
Для завершения введите Ctrl-Z
URL: HTTP://yandex.ru:80/index.html
Invalid URL line
- Программа должна быть нечувствительной к регистру символов протокола. Покрыть тестами
alexey-malov commented
URL: http://yandex:80
Invalid URL line
URL: http://yandex.ru:80
Invalid URL line
URL: http://yandex.ru
Invalid URL line
- Перечисленные выше URL-ы должны считаться валидными. Покрыть тестами
alexey-malov commented
CHttpUrl::CHttpUrl(std::string const & url)
{
regex urlRegex(regexLine);
match_results<const char *> result;
if (!(regex_search(url.c_str(), result, urlRegex)))
{
throw CUrlParsingError("Invalid URL line");
}
m_domain = VerifiedDomain(string (result[2].first, result[2].second));
m_document = VerifiedDocument(string(result[4].first, result[4].second));
m_protocol = GetProtocolFromStr(std::string(result[1].first, result[1].second));
m_port = GetPortFromStr(string(result[3].first, result[3].second));
}
- Прежде чем обращаться к элементам массива, надо проверить, что он содержит нужное количество элементов
alexey-malov commented
unsigned short CHttpUrl::GetPortFromStr(string const & portStr) const
{
int port = 0;
if (portStr.empty())
{
return static_cast<unsigned short>(m_protocol);
}
else try
{
port = stoi(portStr);
}
catch (out_of_range&)
{
throw CUrlParsingError("Port value is out of integer range");
}
return VerifiedPort(port);
}
- Кроме out_of_range могут быть выброшены другие типы исключений функцией stoi.
- Исключения следует ловить по константной ссылке, а не по значению, чтобы избежать срезки (slicing). В редких случаях можно ловить по неконстантной ссылке, если требуется модифицировать пойманный объект исключений перед перевыбросом его.
alexey-malov commented
- Для проверки исключений есть BOOST_CHECK_THROW и BOOST_CHECK_EXCEPTION
http://www.boost.org/doc/libs/1_60_0/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref/assertion_boost_level_exception.html
alexey-malov commented
- can be constructed
alexey-malov commented
BOOST_AUTO_TEST_SUITE(should_return_approppriate_exceptions)
BOOST_AUTO_TEST_CASE(when_can_not_parse_URL_line)
- исключения не возвращаются. Они бросаются и ловятся
alexey-malov commented
- стандартный по-английски будет standard
alexey-malov commented
void VerifyCHttpUrl(const variant<const string&, const UrlParams&> parameters, const UrlParams& expected)
{
optional<CHttpUrl> url;
if (parameters.type() == typeid(string))
{
BOOST_CHECK_NO_THROW(url = CHttpUrl(get<const string&>(parameters)));
VerifyUrlParams(
{ url.get().GetDomain(), url.get().GetDocument(), url.get().GetProtocol(), url.get().GetPort() },
expected);
}
else if (parameters.type() == typeid(UrlParams))
{
UrlParams p = get<UrlParams>(parameters);
if (p.port.is_initialized())
{
BOOST_CHECK_NO_THROW(url = CHttpUrl(p.domain, p.document, p.protocol.get(), p.port.get()));
}
else if (!p.port.is_initialized() && !p.protocol.is_initialized())
{
BOOST_CHECK_NO_THROW(url = CHttpUrl(p.domain, p.document));
}
else
{
BOOST_CHECK_NO_THROW(url = CHttpUrl(p.domain, p.document, p.protocol.get()));
}
VerifyUrlParams(
{ url.get().GetDomain(), url.get().GetDocument(), url.get().GetProtocol(), url.get().GetPort() },
expected);
}
}
- Разбейте на 2 функции. Если хочется проверить работу конструкторов с разными параметрам, передавайте лямбду, конструирующую и возвращающую URL
alexey-malov commented
void VerifyCHttpUrl(const variant<const string&, const UrlParams&> parameters, const UrlParams& expected)
- объекты следует передавать по константной ссылке, если функция не модифицирует их состояние.
Примитивные объекты передавать по константной ссылке выгоды нет, лучше по значению
oMystique commented
- Не совсем понятно для чего нужен файл
CUrlParsingError.cpp
, содержащий следующий контент:
#include "stdafx.h"
#include "CUrlParsingError.h"
oMystique commented
- Стоит выпилить пустую приватную область у данного класса
class CUrlParsingError : public std::invalid_argument
{
public:
using std::invalid_argument::invalid_argument;
private:
};
- Так же, имейте в виду, что внешние инклуды типа
#include "exception"
нужно заключать в <
>
, а не в "
"
, дабы файл не искался в папке проекта.
oMystique commented
- Вместо написания подобного рода велосипедов вполне можно использовать
std::map
, где ключем будет являться строковая константа, а значением - объект типа enum
if (protocol == "http")
{
resultProtocol = Protocol::HTTP;
}
else if (protocol == "https")
{
resultProtocol = Protocol::HTTPS;
}
else if (protocol == "ftp")
{
resultProtocol = Protocol::FTP;
}
Example:
static const std::map<std::string, Protocol> MY_MAP = {
{"http", Protocol::HTTP},
{"ftp", Protocol::FTP},
{...}
}
auto myProtocol = MY_MAP.at("http");
Учесть, что метод .at вполне может выпросить исключение.
В обратную сторону такая тема тоже будет работать.
if (protocol == Protocol::HTTP)
{
result = "http";
}
else if (protocol == Protocol::HTTPS)
{
result = "https";
}
else if (protocol == Protocol::FTP)
{
result = "ftp";
}
oMystique commented
-
boost::to_lower
string CHttpUrl::ToLower(string str) const
{
std::transform(str.begin(), str.end(), str.begin(), tolower);
return str;
}
Кстати говоря. Методы, не использующие фукнционал класса, следует объявлять статическими.
sergeythrees commented
@alexey-malov, @oMystique - Замечания исправлены
alexey-malov commented
void ParseURLsFromStream(istream& input, ostream& output)
{
string urlString;
while (!input.eof())
{
output << " > ";
getline(input, urlString);
if (urlString.empty())
{
continue;
}
try
{
CHttpUrl url(urlString);
output
<< "URL: " << url.GetURL() << endl
<< "HOST: " << url.GetDomain() << endl
<< "PORT: " << url.GetPort() << endl
<< "DOC: " << url.GetDocument() << endl;
}
catch (const invalid_argument &ex)
{
cerr << ex.what() << endl;
}
output << endl;
}
}
- Возможен бесконечный цикл. и вот почему: http://www.viva64.com/en/w/V663/print/