sergeythrees/Object-Oriented-Programming

Замечания по CHttpURL

Closed this issue · 16 comments

alexey-malov commented

image

  • Часть веток не покрыта тестами, либо код неиспользуемый
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
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;
	}
}