Skip to content

Conversation

@MinyazevR
Copy link
Owner

No description provided.

using System;
using Stack;

namespace StackCalculator;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namespace лучше в самом начале писать

/// </summary>
public class Calculator
{
private IStack<float> Stack = new StackOnLists<float>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// <returns>Expression value</returns>
public float CountTheExpressionInPostfixForm(string[] inputString)
{
int number = 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Слишком рано объявлен. Надо его объявить в месте инициализации: if (int.TryParse(inputString[i], out int number))

/// <summary>
/// A class representing the stack interface
/// </summary>
abstract public class IStack<T>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это должен был быть интерфейс, а не абстрактный класс

/// Function that returns the number of elements in the stack
/// </summary>
/// <returns>Number of elements in stack</returns>
abstract public int ReturnNumberOfElements();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Идеологически правильнее NumberOfElements();, "Return" тут несколько избыточно.

numberOfElements--;
return value;
}
return default(T);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return default(T);
return default;

Comment on lines 81 to 85
if (head == null)
{
throw new StackException("Stack is empty");
}
return head == null ? default(T) : head.Value;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну...

Comment on lines 13 to 18
private static IEnumerable<TestCaseData> Stacks
=> new TestCaseData[]
{
new TestCaseData(new StackOnArray<int>()),
new TestCaseData(new StackOnLists<int>()),
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static IEnumerable<TestCaseData> Stacks
=> new TestCaseData[]
{
new TestCaseData(new StackOnArray<int>()),
new TestCaseData(new StackOnLists<int>()),
};
private static IEnumerable<TestCaseData> Stacks
=> new TestCaseData[]
{
new TestCaseData(new StackOnArray<int>()),
new TestCaseData(new StackOnLists<int>()),
};

public void CheckTopOfTheStackAfterPush(IStack<int> stack)
{
stack?.Push(1);
Assert.AreEqual(stack?.ReturnTopOfTheStack(), 1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Наоборот, сначала Expected, затем Actual. Иначе если тест не пройдёт, сообщение об ошибке будет лживым.

[TestCaseSource(nameof(Stacks))]
public void CheckTopOfEmptyStack(IStack<int> stack)
{
var exception = Assert.Throws<StackException>(() => stack?.ReturnTopOfTheStack());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"?" тут не нужен, стек гарантированно не null

Copy link

@yurii-litvinov yurii-litvinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Неаккуратно, но по существу всё ок, зачтена.

Comment on lines +65 to +68
}


[TestCaseSource(nameof(Stacks))]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
[TestCaseSource(nameof(Stacks))]
}
[TestCaseSource(nameof(Stacks))]

/// <summary>
/// A class for testing a stack calculator
/// </summary>
public class TestsStackCalculator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public class TestsStackCalculator
public class StackCalculatorTests

Так каноничнее

public void ShouldThrowsDivideByZeroExceptionWhenDivideByZero(IStack<float> stack)
{
string[] firstArray = { "123", "0", "/" };
Assert.Throws<System.DivideByZeroException>(() => stackCalculator?.CountTheExpressionInPostfixForm(firstArray, stack));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вообще, калькулятор должен принимать строку, а не массив, но ладно :)

public void ShouldThrowsInvalidCharacterExceptionWhenExpressionContainsInvalidCharacter(IStack<float> stack)
{
string[] firstArray = { "123", "0", "a" };
Assert.Throws<InvalidCharacterException> (() => stackCalculator?.CountTheExpressionInPostfixForm(firstArray, stack));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assert.Throws<InvalidCharacterException> (() => stackCalculator?.CountTheExpressionInPostfixForm(firstArray, stack));
Assert.Throws<InvalidCharacterException>(() => stackCalculator?.CountTheExpressionInPostfixForm(firstArray, stack));

Comment on lines +47 to +50
}


[TestCaseSource(nameof(Stacks))]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
[TestCaseSource(nameof(Stacks))]
}
[TestCaseSource(nameof(Stacks))]

Copy link

@yurii-litvinov yurii-litvinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сам калькулятор со стеками ещё всё-таки надо немного поправить

/// Function for counting expressions in postfix form
/// </summary>
/// <returns>Expression value</returns>
public float CountTheExpressionInPostfixForm(string[] inputString, IStack<float> stack)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нет-нет, хочется, чтобы калькулятор принимал строку, а не массив строк

Comment on lines +41 to +44
catch (StackIsEmptyException)
{
throw;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Если вы думаете, что ваша жизнь бессмысленна, посмотрите на эти строки кода :)

/// </summary>
public class IncorrectExpressionException : Exception
{
public IncorrectExpressionException() : base() { }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это тоже нечто ненужное. Если в классе не объявлен ни один конструктор, компилятор сам генерирует как раз это.

{
if (values != null && numberOfElements == values.Length)
{
Array.Resize(ref values, values.Length + 20);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лучше увеличивать сразу вдвое. Иначе как у Вас операция Push работает в среднем за линию, а если стек растёт вдвое, то в среднем за константу.

}

numberOfElements++;
if (values == null)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кажется, nullability-анализ гарантирует, что values не null, так что это довольно бессмысленная проверка

}

T topOfSTack = values[numberOfElements - 1];
Array.Clear(values, numberOfElements - 1, 1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это хитрый способ записать values[numberOfElements - 1] = default;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants