Любые варианты улучшения кода
Есть 2 задачи на ulern.me: Практика «Limited Size Stack» и Практика «Отмена», обе ссылаются на один и тот же проект, поставленную задачу выполнил, все тесты прошли успешно.
Интересно можно ли как-нибудь улучшить/оптимизировать мое решение (новичка). Так же повысить читаемость (по возможности) и по сути, сделать код максимально правильным.
Если есть - найти ошибки.
Просьба писать подробно.
Содержимое Limited Size Stack.cs:
using System;
using System.Collections.Generic;
namespace LimitedSizeStack;
public class LimitedSizeStack<T>
{
public int Count { get { return StackList.Count; } }
private int MaxCount { get; set; }
public LinkedList<T> StackList { get; private set; }
private bool IsFullStack { get { return Count > MaxCount; } }
private bool IsEmptyStack { get { return Count == 0; ; } }
public LimitedSizeStack(int undoLimit)
{
StackList = new();
MaxCount = undoLimit;
}
public void Push(T item)
{
StackList.AddFirst(item);
if (IsFullStack)
StackList.RemoveLast();
}
public T Pop()
{
if (IsEmptyStack)
throw new InvalidOperationException("Stack is empty");
else
{
var removedItem = StackList.First ?? throw new ArgumentException();
StackList.RemoveFirst();
return removedItem.Value;
}
}
}
Содержимое ListModel.cs:
using System;
using System.Collections.Generic;
namespace LimitedSizeStack;
public class ListModel<T>
{
public List<T> Items { get; }
public LimitedSizeStack<Tuple<string, int, T>> HistoryActions { get; private set; }
private bool HistoryActionsNoEmpty { get { return HistoryActions.Count > 0; } }
public ListModel(int undoLimit) : this(new List<T>(), undoLimit)
{
}
public ListModel(List<T> items, int undoLimit)
{
HistoryActions = new LimitedSizeStack<Tuple<string, int, T>>(undoLimit);
Items = items;
}
public Tuple<string, int, T> GetValue(string action, int index, T item)
{
Tuple<string, int, T> value = new (action, index, item);
return value;
}
public void AddItem(T item)
{
HistoryActions.Push(GetValue("AddItem", -1, item));
Items.Add(item);
}
public void RemoveItem(int index)
{
HistoryActions.Push(GetValue("RemoveItem", index, Items[index]));
Items.RemoveAt(index);
}
public bool CanUndo()
{
return HistoryActionsNoEmpty;
}
public void Undo()
{
if (CanUndo())
{
var lastEdit = HistoryActions.Pop();
if (lastEdit.Item1 == "AddItem")
Items.RemoveAt(Items.Count - 1);
else
Items.Insert(lastEdit.Item2, lastEdit.Item3);
}
}
}
Ответы (1 шт):
public int Count { get { return StackList.Count; } }
можно юзать лямбда-стайл
public int Count => StackList.Count;
private int MaxCount { get; set; }
Зачем тут сеттор? Вы его не используете, у Вас MaxCount ридонли, и это хорошо, но нужно подчеркнуть, убрав сеттор. В конструкторе, он сможет задать значение и без него.
public LinkedList<T> StackList { get; private set; }
опять ненужный сеттор, и непонятно зачем публично выставлять потроха, убирайте public. Ещё одно замечание - перемешаны public и private члены, а приятнее когда они сгруппированы.
private bool IsFullStack { get { return Count > MaxCount; } }
private bool IsEmptyStack { get { return Count == 0; ; } }
лямбда-стайл?
public LimitedSizeStack(int undoLimit)
{
что за undoLimit? У вас протекла абстракция похоже (: переименуйте в maxCount
StackList = new();
MaxCount = undoLimit;
}
можно заинициализировать StackList инлайн
private readonly LinkedList<T> StackList = new();
public void Push(T item)
{
StackList.AddFirst(item);
if (IsFullStack)
StackList.RemoveLast();
}
Иногда, в код-стайлах принято запрещать отсутствие фигурных скобочек после условия.
public T Pop()
{
if (IsEmptyStack)
throw new InvalidOperationException("Stack is empty");
else
{
var removedItem = StackList.First ?? throw new ArgumentException();
StackList.RemoveFirst();
return removedItem.Value;
}
}
}
лишняя пустая строка после else {.
Странная запоздалая проверка StackList.First на null и тем более не очень ясно с каким аргументом проблема, раз у нас ArgumentException. Не бросайте ArgumentException с пустым конструктором. И мне кажется, вы дважды пытаетесь проверить одно и тоже.
Т.к. вы уверены что в First null не может быть, и у вас включены null refrence type, укажите это явно
var removedItem = StackList.First!;
Нет метода Peek, который возвращает элемент на вершине стека, но не снимает его. Этот метод характерен для интерфейса стеков.
Почему выбран LinkedList, а не собственно Stack ? Решение-то хорошее алгоритмически, но со Stack было бы и более читаемо(что важно) и лучше по перформансу (что скорее преждевременная оптимизация). Если это в учебных целях, то вопрос снимается, но в продакшене должны быть чёткие основания, если вдруг что-то велосипедится, вместо чтобы взять наиболее подходящее решение.
Вы отредактировали код в вопросе и теперь у вам там рид-онли поля, а не свойства. Поля принято именовать или с маленькой буквы, или с префикса _ , или с префикса m_, но точно не с заглавной буквы.