Любые варианты улучшения кода

Есть 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 шт):

Автор решения: 4per
    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_, но точно не с заглавной буквы.

→ Ссылка