Number 2 #30
Number 2 #30
Conversation
CyclicList/CyclicList/CyclicList.h
Outdated
| // Structure containing pointers to the "first" and "last" list item | ||
| typedef struct List | ||
| { | ||
| struct ListElement* head; | ||
| struct ListElement* tail; | ||
| } List; | ||
|
|
||
| // Structure containing pointers to the next and previous list item and a value variable for list items | ||
| typedef struct ListElement | ||
| { | ||
| int value; | ||
| struct ListElement* next; | ||
| struct ListElement* previous; | ||
| } ListElement; | ||
|
|
||
| // Structure containing a pointer to the position of a list item | ||
| typedef struct Position | ||
| { | ||
| ListElement* position; | ||
| } Position; |
There was a problem hiding this comment.
Тут тоже, надо "упаковать" это всё в АТД "Циклический список", который бы следил за своими инвариантами, не раскрывал своего внутреннего устройства и был переиспользуемым. Конечно, отвалятся тесты, но они и не должны тестировать внутреннее устройство, они должны тестировать наблюдаемое поведение (иначе при изменении чего-то в реализации придётся и тесты переписывать).
| next(firstPosition); | ||
| counter++; | ||
| } | ||
| const int answer = newList->head->value; |
There was a problem hiding this comment.
Стоит тут функцию для доступа к значению в голове сделать. Незачем лезть в личную жизнь списка.
| list->head = list->head->next; | ||
| free(position); | ||
| position = list->head; | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CyclicList/CyclicList/CyclicList.c
Outdated
| list->tail = newElement; | ||
| list->tail->next = list->head; | ||
| list->tail->previous = list->head; | ||
| list->head->next = list->tail; |
There was a problem hiding this comment.
Это излишне. list->head и list->tail указывают на один и тот же newElement, так что эта строчка повторяет то, что уже сделано двумя строками выше.
| } | ||
| newElement->previous = list->tail; | ||
| list->tail->next = newElement; | ||
| newElement->next = list->head; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| position->position->next->previous = position->position->previous; | ||
| position->position->previous = position->position; | ||
| position->position = position->position->next; | ||
| } No newline at end of file |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CyclicList/CyclicList/CyclicList.h
Outdated
| #include <stdbool.h> | ||
| #include <malloc.h> | ||
|
|
||
| // Structure containing pointers to the "first" and "last" list item |
There was a problem hiding this comment.
Не-а. Structure that represents cyclic list, скорее. Что она содержит --- это детали реализации, которые могут меняться в процессе рефакторинга или оптимизации программы. Почитайте где-нибудь про абстрактные типы данных (например, Ахо и др., "Структуры данных и алгоритмы", эта часть курса в целом по ней), мне кажется, у Вас с принципом абстракции проблемы, а это самый важный принцип в программировании вообще.
CyclicList/CyclicList/CyclicList.h
Outdated
| typedef struct List List; | ||
|
|
||
| // Structure containing pointers to the next and previous list item and a value variable for list items | ||
| typedef struct ListElement ListElement; |
There was a problem hiding this comment.
Не, ListElement тоже надо спрятать. Циклический список даже проще реализовать в массиве со взятием индекса по модулю
| return 0; | ||
| } | ||
| int counter = 1; | ||
| while (returnFirstElement(newList) != returnLastElement(newList)) |
There was a problem hiding this comment.
Сделайте лучше функцию length для списка или даже "isOneElement" (что хуже, потому что она применима разве что к этой задаче, но проще)
CyclicList/CyclicList/CyclicList.c
Outdated
| return position; | ||
| } | ||
|
|
||
| void invariant(ListElement* element) |
There was a problem hiding this comment.
Не, эту функцию стоило бы назвать как-то более осмысленно (например, то же attach, или link, или что-то такое).
CyclicList/CyclicList/CyclicList.c
Outdated
| { | ||
| list->tail = NULL; | ||
| list->head = NULL; | ||
| free(list->head); |
There was a problem hiding this comment.
Поздно, тут list->head уже NULL :)
| (*position)->position->previous = (*position)->position->previous->previous; | ||
| free(element); | ||
| invariant((*position)->position); | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
yurii-litvinov
left a comment
yurii-litvinov
left a comment
There was a problem hiding this comment.
Комментарии к коммитам в духе "changing functions" нельзя, напишите лучше, что именно changing и какие именно функции. Вообще, вот рекомендации по хорошим сообщениям к коммитам: https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53.
Но в остальном всё хорошо, зачтена
| // Structure that represents cyclic list | ||
| typedef struct List List; | ||
|
|
||
| // Structure for implementing a cyclic list |
There was a problem hiding this comment.
Неправда :) Откомментил в предыдущей задаче аналогичную проблему
| free(position); | ||
| position = list->head; | ||
| } | ||
| list->size = 0; |
There was a problem hiding this comment.
Это не нужно. Мы двумя строками ниже удаляем весь list. Всё равно как покрасить дом перед тем, как его снести.
| ListElement* element = list->tail; | ||
| list->tail = list->tail->previous; | ||
| list->tail->next = list->head; | ||
| attach(list->tail); |
There was a problem hiding this comment.
Кажется, можно было бы вынести более общую функцию, типа "удалить между", и использовать её тут в трёх местах.
2 participants
…oblem