Замечания по программе Radix

[alexey-malov]

• Тесты должны запускаться во всех конфигурациях и платформах

[alexey-malov]

C:\teaching\oop-labs\ler\OopLabs\lab1\Radix\x64\Debug>radix.exe 16 10 -80000000
-0

• Некорректно обрабатывается число INT_MIN

[alexey-malov]

OopLabs/lab1/Radix/Radix.cpp

Lines 131 to 142 in 8d63598

	int main(int argc, char* argv[])
	{
	if (argc != 4)
	{
	std::cout << "Invalid argument count\n"
	<< "Usage: radix.exe <source notation> <destination notation> <value>\n";
	return 1;
	}
	string source = argv[1];
	string destination = argv[2];
	string value = argv[3];

• Выделите функцию парсинга командной строки, возвращающую опциональную структуру с полями: исходная и конечная системы счисления (в виде чисел), строка для перевода

[alexey-malov]

OopLabs/lab1/Radix/Radix.cpp

Line 97 in 8d63598

int GetInt(string& source, string& destination, string& value, bool& wasError, int& destinationInt)

• Начальная и конечная системы счисления должны принимать в виде int, а строк

• объекты следует передавать по константной ссылке, если функция не модифицирует их состояние.
  Примитивные объекты передавать по константной ссылке выгоды нет, лучше по значению

[alexey-malov]

OopLabs/lab1/Radix/Radix.cpp

Lines 72 to 95 in 8d63598

	bool CheckValue(int& arg1, int& arg2, string& arg3)
	{
	if (arg1 != -1 && arg2 != -1)
	{
	string alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
	int negativeSwitch = 0;
	if (arg3[0] == '-')
	{
	negativeSwitch = 1;
	}
	if (arg3.substr(negativeSwitch, arg3.size()).find_first_not_of(alphabet.substr(0, arg1)) == string::npos)
	{
	return true;
	}
	else
	{
	return false;
	}
	}
	else
	{
	return false;
	}
	}

• Что такое arg1, arg2, arg3?

• Почему значения передаются по неконстантной ссылке?

• Сложно. Не ясно, что делает функция

[alexey-malov]

• stoi может выбросить исключение. Программа должна обрабатывать исключения

[alexey-malov]

OopLabs/lab1/Radix/Radix.cpp

Lines 27 to 31 in 8d63598

	if (n < 0)
	{
	negativeSwitch = "-";
	n = abs(n);
	}

• Для числа INT_MIN нет абсолютного значения

[alexey-malov]

OopLabs/lab1/Radix/Radix.cpp

Lines 44 to 47 in 8d63598

	if (result == "")
	{
	result = "0";
	}

• Выглядит как костыль, которого можно было бы избежать при использовании цикла do-while

[alexey-malov]

OopLabs/lab1/Radix/Radix.cpp

Lines 9 to 22 in 8d63598

	int StringToInt(const string& str, int radix, bool& wasError)
	{
	try
	{
	int result = stoi(str, nullptr, radix);
	return result;
	}
	catch (out_of_range & err)
	{
	cout << err.what() << endl;
	wasError = true;
	return 0;
	}
	}

• Преобразование следует реализовать самостоятельно

[alexey-malov]

• GetInt выглядит сложно

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Line 102 in d56a9fc

int StringToInt(const string str, int radix, bool& wasError)

• объекты следует передавать по константной ссылке, если функция не модифицирует их состояние.
  Примитивные объекты передавать по константной ссылке выгоды нет, лучше по значению

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Line 173 in d56a9fc

bool CheckArgs(int source, int destination, const string value, bool& wasError)

• объекты следует передавать по константной ссылке, если функция не модифицирует их состояние.
  Примитивные объекты передавать по константной ссылке выгоды нет, лучше по значению

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Line 202 in d56a9fc

bool ConvertValue(int source, int destination, const string value)

• объекты следует передавать по константной ссылке, если функция не модифицирует их состояние.
  Примитивные объекты передавать по константной ссылке выгоды нет, лучше по значению

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Lines 102 to 114 in d56a9fc

	int StringToInt(const string str, int radix, bool& wasError)
	{
	const int maxRadix = ('Z' - 'A') + 11;
	size_t i = 0;
	int number = 0;
	int n = 0;
	bool negativeSwitch = false;
	if (str[0] == '-')
	{
	negativeSwitch = true;
	i = 1;
	}
	while (i <= str.length() - 1 && !wasError)

• Некорректная обработка пустой строки (в двух местах)

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Lines 116 to 123 in d56a9fc

	if (str[i] >= '0' && str[i] <= '9')
	{
	number = str[i] - '0';
	}
	else
	{
	number = str[i] - 'A' + 10;
	}

• Выделить в функцию, добавить обработку ошибок (выход цифр за пределы диапазона)

• number - неудачное имя. digit (цифра) больше подходит

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Lines 149 to 156 in d56a9fc

	if (absN > 9)
	{
	result = (char)(absN + 'A' - 10) + result;
	}
	else
	{
	result = (char)(absN + '0') + result;
	}

• Выделить в функцию

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Lines 178 to 187 in d56a9fc

	int negativeSwitch = 0;
	if (value[0] == '-')
	{
	negativeSwitch = 1;
	}
	if (value.substr(negativeSwitch, value.size()).find_first_not_of(alphabet.substr(0, source)) == string::npos)
	{
	return true;
	}
	else

• negativeSwitch - неудачное имя

• Сделать без извлечения подстроки

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Line 113 in 92c2532

int extractDigit(char symbol, int radix, bool& wasError)

• Программа не соответствует принятому стилю кодирования. Ознакомьтесь с правилами именования переменных, функций, классов, расстановкой скобок, знаков препинания и отступов.

• Не везде инициализируется параметр wasError

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Lines 144 to 177 in 92c2532

	int StringToInt(const string& str, int radix, bool& wasError)
	{
	const int maxRadix = ('Z' - 'A') + 11;
	size_t i = 0;
	int digit = 0;
	int n = 0;
	bool isNegative = false;
	if (str.length() == 0) {
	wasError = true;
	cout << "Value cannot be empty" << endl;
	return 0;
	}
	if (str[0] == '-')
	{
	isNegative = true;
	isNegative = true;
	i = 1;
	}
	while (i <= str.length() - 1 && !wasError)
	{
	digit = extractDigit(str[i], radix, wasError);
	if (isNegative)
	{
	digit = -digit;
	}
	if (!SafeMult(n, radix, n) || !SafeAdd(n, digit, n))
	{
	wasError = true;
	cout << "Overflow error" << endl;
	}
	i++;
	}
	return n;
	}

• выходной параметр wasError может быть неинициализирован
• Переменные следует объявлять как можно ближе к месту их первого использования (желательно, совмещая с инициализацией).
• В isNegative дважды записывается значение
• Эта функция не должна ничего записывать в stdout

OopLabs/lab1/Radix/Radix/Radix.cpp

Lines 159 to 174 in 1050670

	int digit = 0;
	int n = 0;
	while (i <= str.length() - 1 && !wasError)
	{
	digit = ExtractDigit(str[i], radix, wasError);
	if (isNegative)
	{
	digit = -digit;
	}
	if (!SafeMult(n, radix, n) || !SafeAdd(n, digit, n))
	{
	wasError = true;
	}
	i++;
	}
	return n;

• Если переменная используется только внутри тела цикла и ее значение на текущей итерации не зависит от значения на предыдущей, то такую переменную следует объявить внутри цикла. Тогда ее область видимости будет ограничена телом цикла, а не до конца функции, как у вас. Исключение: создание и разрушение переменной на каждой итерации цикла является ресурсоемкой операцией, оказывающей влияние на эффективность работы приложения. В этом случае переменную можно объявить вне тела цикла.

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Lines 190 to 194 in 92c2532

	string IntToString(int n, int radix) {
	string result = "";
	string isNegative = "";
	int absN = 0;
	if (n < 0)

• Переменные следует объявлять как можно ближе к месту их первого использования (желательно, совмещая с инициализацией).
• Для объявления пустой строки не нужно писать = "". С этим справится конструктор по умолчанию

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Lines 220 to 239 in 92c2532

	bool ConvertValue(int source, int destination, const string& value)
	{
	bool wasError = false;
	if (CheckNotation(source) && CheckNotation(destination))
	{
	int number = StringToInt(value, source, wasError);
	if (wasError)
	{
	return false;
	}
	string result = IntToString(number, destination);
	cout << result << endl;
	return true;
	}
	cout << "Error in 1st or 2nd argument" << endl;
	return false;
	}

• Отделите перевод числа из одной системы счисления в другую от вывода в cout. Пусть в cout выводит функция main
• Ошибки тоже внутри этой функции выводиться не должны

[alexey-malov]

OopLabs/lab1/Radix/Radix/Radix.cpp

Lines 177 to 187 in 1050670

	char extractChar(int value)
	{
	if (value > 9)
	{
	return (char)(value + 'A' - 10);
	}
	else
	{
	return (char)(value + '0');
	}
	}

• Программа не соответствует принятому стилю кодирования. Ознакомьтесь с правилами именования переменных, функций, классов, расстановкой скобок, знаков препинания и отступов.

[alexey-malov]

Переменные можно и нужно объявлять как можно ближе к месту их первого использования. Причем если переменная используется только внутри цикла и не должна сохранять свое значение между итерациями, то лучше объявить ее внутри цикла.

[alexey-malov]

Кроме 1 лабораторной работы есть и 6 других. Если они не будут сданы, вы не будете допущены до экзамена