Более подробные отзывы в нижней части этого поста, но для начала просто несколько встроенных комментариев и изменений в коде:
struct Node // Why doesn't this have a constructor initializing the members?
{
int value;
Node *next;
} *lnFirst;
ListNode::ListNode() : lnFirst(NULL) {} // Use initializer lists instead of initializing members in the ctor body. It's cleaner, more efficient and may avoid some nasty bugs (because otherwise the member gets default-initialized *first*, and then assigned to in the body)
int ListNode::Length()
{
int intCount = 0;
for( Node* lnTemp=lnFirst ; lnTemp != NULL ; lnTemp = lnTemp->next ) // Create the loop iteration variable lnTemp here, in the loop, not at the start of the function
{
intCount++;
}
return intCount;
}
void ListNode::DisplayList()
{
for(Node* lnTemp = lnFirst ; lnTemp != NULL ; lnTemp = lnTemp->next ) // And again, initialize the loop variable in the loop
cout<<endl<<lnTemp->value; // Not a huge deal, but endl flushes the stream as well as inserting a newline. That can be needlessly slow. So you might want to just use "\n" in cases where you don't need the flushing behavior.
}
void ListNode::Insert(int num)
{
// Node *lnCurrent, *lnNew; // Very subjective, but I prefer not declaring multiple variables on the same line, because the syntax if they're pointers can be surprising (You got it right, but a lot of people would write Node* lnCurrent, lnView, which would make lnView not a pointer. I find it clearer to just give ecah variable a separate line:
if( lnFirst == NULL )
{
// lnFirst = new Node;
// lnFirst->value = num;
// lnFirst->next = NULL;
lnFirst = new Node(num); // Make a constructor which initializes next to NULL, and sets value = num. Just like you would in other languages. ;)
}
else
{
Node* lnCurrent = lnFirst; // Don't declare variables until you need them. Both to improve readability, and because C++ distinguishes between initialization and assignment, so in some cases, default-initialization followed by assigment may not be the same as just initializing with the desired value.
while( lnCurrent->next != NULL )
lnCurrent = lnCurrent->next;
Node* lnNew = new Node(num); // Again, let a constructor do the work.
lnCurrent->next = lnNew;
}
}
bool ListNode::Contains(int num)
{
bool boolDoesContain = false;
// Node *lnTemp,*lnCurrent; // Again, don't initialize variables at the start of the function if they're not needed
Node* lnCurrent = lnFirst;
// lnTemp = lnCurrent;
while( lnCurrent!=NULL )
{
if( lnCurrent->value == num )
{
// boolDoesContain = true;
// return boolDoesContain;
return true; // Just return directly, and remove the boolDoesContain variable. Alternatively, set boolDoesContain to true, and then break out of the loop without returning, so you have a single exit point from the function. Both approaches have their merits, but setting a variable you don't need, and then returning is silly. ;)
}
// Node* lnTemp = lnCurrent; // you don't actually use lnTemp for anything, it seems
lnCurrent = lnCurrent->next;
}
// return boolDoesContain;
return false; // just return false. If you get this far, it must be because you haven't found a match, so boolDoesContain can only be false anyway.
}
int ListNode::GetValue(int num)
{
// Node *lnTemp;
int intCount = 1; // Wouldn't most people expect this indexing to be zero-based?
for( Node* lnTemp=lnFirst; lnTemp != NULL; lnTemp = lnTemp->next )
{
if (intCount == num)
{
return lnTemp->value;
}
intCount++;
}
}
Теперь пара общих комментариев. (Я собираюсь игнорировать, неправильно ли вы поняли назначение, и сосредоточиться на опубликованном вами коде)
Во-первых, венгерская запись: не надо. Сначала вызывайте указатели вашего узла, временные и любые другие, без префикса 'ln'. Вызовите вашу переменную bool doContain без ненужного префикса bool.
Во-вторых, как я пытался сделать в отредактированном коде, создавайте переменные только тогда, когда они вам нужны. C раньше требовал объявления переменных в верхней части блока, но C ++ никогда этого не делал.
В-третьих, вам не нужно возвращать 0 в конце основной функции. Main - это особый случай, когда, если он достигает конца функции, он автоматически возвращает 0.
В-четвертых, у нас есть большая неприятная проблема: управление памятью.
Вы выделяете память, которая никогда не освобождается. Поскольку у вас нет функции RemoveNode, это может показаться спорным вопросом, но что происходит, когда весь список выходит из области видимости и удаляется? Ни один из его узлов не удален, потому что весь список содержит набор указателей, и он не вызывает автоматически их удаление.
Поэтому, по крайней мере, вам нужен деструктор в самом классе списка, чтобы при удалении списка он обязательно удалял все свои дочерние узлы.
Это должно обрабатывать простой случай по умолчанию, когда вы создаете список, добавляете к нему узлы и удаляете список.
Следующая большая проблема, что если я скопирую список?
int main(){
ListNode list;
list.Insert(1);
list.Insert(2);
list.Insert(3);
}
ListNode list2 = list;
Ваш код взрывается. Оба списка теперь указывают на одинаковые узлы вместо того, чтобы делать копии узлов. Добавление узла в один список также сделает его обнаруженным в другом.
И прежде чем вы заявите, что «это особенность, а не ошибка»;), подумайте, что происходит, когда один из списков удаляется сейчас .
Предположим, что list2 удаляется первым. С деструктором, который мы только что определили, он удаляет три узла и возвращает.
Теперь список указывает на узлы, которые были удалены. Доступ к ним является неопределенным поведением и вполне может привести к сбою. Допустим, мы не получаем к ним доступ, вместо этого мы просто быстро удаляем и этот список.
Ой, это означает, что мы пытаемся удалить дочерние узлы, которые уже были удалены. Это определенно звучит как авария.
Итак, чтобы исправить это, ваш класс ListNode должен реализовать две дополнительные функции, конструктор копирования и оператор присвоения:
ListNode(const ListNode& other);
ListNode& operator==(const ListNode& other);
Эти два должны гарантировать, что когда все данные скопированы из «другого». Для каждого узла в «другом» вы должны выделить новый узел в текущем списке, а не просто указывать в обоих списках один и тот же узел. (Это означает, что классу узла, скорее всего, также нужен конструктор копирования, как минимум).
Это основной способ управления памятью, и его важно понять, потому что в противном случае вы испортите . ;)