Skip to content

Хранение логов#4

Open
AHDEPCEH wants to merge 7 commits intomainfrom
task4
Open

Хранение логов#4
AHDEPCEH wants to merge 7 commits intomainfrom
task4

Conversation

@AHDEPCEH
Copy link
Copy Markdown
Owner

No description provided.

Copy link
Copy Markdown

@vpyzhyanov vpyzhyanov left a comment

Choose a reason for hiding this comment

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

Решение тянет на 8 баллов.

}
int indexTo = Math.min(startFrom + count, m_messages.size());
return m_messages.subList(startFrom, indexTo);
return m_messages.iterator(startFrom, indexTo);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

предыдущее имя было лучше

public Iterable<T> iterator(int startIndex, int endIndex) {
lock.lock();
try {
return new Iterable<>() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

лучше не писать всё в одной куче, а вынести в отдельный класс (можно даже внутри текущего класса)


@Override
public T next() {
T element = buffer[current];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Но требование о нерушимости итератора вы не выполнили. Во время чтения итератора, если добавить много элементов, то итератор начнёт выдавать уже другие данные. Это происходит потому, что все данные берутся из одной корзины.

На самом деле структура данных, которую выбрали вы для этого не подходит.

Copy link
Copy Markdown

@vpyzhyanov vpyzhyanov left a comment

Choose a reason for hiding this comment

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

  1. Не решена проблема утечки памяти
  2. Структура данных не выполняет базовые требования задачи

* Итерируемый лист по записям из сегмента
* @param <T>
*/
class myList<T> implements Iterable<T> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Имена классов должны всегда начинаться с большой буквы

@@ -9,53 +9,52 @@
* удерживаемыми в памяти)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

А где исправление этого пункта?

*/
class Segment<T>{
public Segment<T> next;
public List<T> entries = new LinkedList<>();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  1. с массивом было бы намного быстрее
  2. CRITICAL Насколько я вижу вытеснения старых записей не происходит. А это, между прочим, основное требование этой задачи

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Попытка хорошая

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Сегменты удаляются - это верно. Но вот сегмент состоит не из одной записи. А эти записи внутри сегмента не удаляются. Поэтому может так случится, что записей будет больше, чем максимальная длина коллекции.

@AHDEPCEH
Copy link
Copy Markdown
Owner Author

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

@vpyzhyanov
Copy link
Copy Markdown

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

прошу писать вопросы и комментарии к конкретному замечанию, а не отдельно

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.

2 participants