0

Есть 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 => _StackList.Count;

    private bool IsFullStack => Count > _MaxCount;
    private bool IsEmptyStack => Count == 0;
    private readonly int _MaxCount;
    private readonly LinkedList<T> _StackList = new();

    public LimitedSizeStack(int maxCount)
    {
        _MaxCount = maxCount;
    }

    public void Push(T item)
    {
        _StackList.AddFirst(item);
        if (IsFullStack)
            _StackList.RemoveLast();
    }

    public T Pop()
    {
        if (IsEmptyStack)
            throw new InvalidOperationException("Stack is empty");
        var removedItem = _StackList.First!;
        _StackList.RemoveFirst();
        return removedItem.Value;
    }
}

Содержимое ListModel.cs:

using System;
using System.Collections.Generic;

namespace LimitedSizeStack;
public class ListModel<T> 
{
    private bool HistoryActionsNoEmpty => _HistoryActions.Count > 0;

    public readonly List<T> _Items;
    public readonly LimitedSizeStack<Tuple<string, int, T>> _HistoryActions;

    public ListModel(int undoLimit) : this(new List<T>(), undoLimit)
    {
    }

    public ListModel(List<T> items, int undoLimit)
    {
        _HistoryActions = new(undoLimit);
        _Items = items;
    }

    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);
        }
    }

    private static Tuple<string, int, T> GetValue(string action, int index, T item)
    {
        return new(action, index, item);
    }
}
2
  • 2
    Если хотите улучшить, то что именно? Если считаете что нужно повысить читаемость, что именно плохо читается? Максимально правильного кода не существует. Можно наворотить нечиталбельного бреда и он будет работать быстрее пули, а можно наоборот, сделать тормоз в 3 строки кода. Понятие "правильный код" абстрактно. Если рассуждать про поверхностные критерии качества, то код должен быть понятным. Мне он понятный, но написал бы я по-своему. Одна только непонятность есть, почему метод GetValue публичный?
    – aepot
    1 июл в 22:59
  • @aepot общую мысль понял, по поводу метода GetValue тоже верное замечание, исправил. Благодарю
    – funny110
    2 июл в 6:10

1 ответ 1

2
    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_, но точно не с заглавной буквы.

5
  • Большую часть понял, от себя сделал private readonly int MaxCount; и так же с StackList. Вообще не понял как инициализировать инлайн. Так же, проверка Stack.First на null сделана, т.к VS почему то ругается на removedItem в строке return removedItem.Value; CS8602: разыменование пустой строки, хотя есть проверка на пустую строку, т.к StackList.First пустой только при Count = 0, но при таком условии вылетит InvalidOperationException("Stack is empty"). Не понял зачем нужен метод Peek и что там должно быть.
    – funny110
    2 июл в 7:00
  • LinkedList выбран по подсказкам на Ulearn, изначально сделал свой список, но реализовал другой подход и как следствие не проходились тесты на Ulearn и решил сделать на LinkedList. Не Stack т.к по сути задание было сделать этот самый Stack.
    – funny110
    2 июл в 7:02
  • @funny110 ещё этот буфер действий можно реализовать с помощью простого массива и двух интов. Не буду рассказывать, попробуйте сами догадаться, если интересно.
    – aepot
    2 июл в 9:08
  • @funny110 дополнил по всем замечания из вашего комента
    – 4per
    2 июл в 14:22
  • @4per огромное спасибо за ответ и разъяснения, по примеру с LimitedSizeStack.cs сделал и ListModel.cs, так же честно не знал про возможность поставить ! при присвоении значения.
    – funny110
    2 июл в 17:19

Ваш ответ

Нажимая на кнопку «Отправить ответ», вы соглашаетесь с нашими условиями использования и подтверждаете, что прочитали и поняли наши политику конфиденциальности и нормы поведения.

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.