Besoin de conseils

Bonjour a tous, j'aimerais si possible que vous jetiez un petit coup d'oeil a mon code, c'est un de mes premiers codes en C++ donc svp ne soyez pas trop critiques. J'aimerais en revanche beneficier de votre experience pour ameliorer mon code, corriger ses erreurs, ameliorer le style etc...

En gros c'est un exercice a faire pour mon universite et ils ont beaucoup insiste sur les principes suivants : encapsulation, utilisation de const imperative dans les headers de fonction qui ne sont pas sensees modifier le code, etc.

L'exercice est l'implementation d'une Liste Chainee d'objets string.

Merci d'avance pour tous vos conseils, corrections ou autres!!

MyLinkedList.h

#ifndef _MyLinkedList_h
#define _MyLinkedList_h
class MyLinkedList
{
public:
MyLinkedList();
~MyLinkedList();
void Add (const std::string &toAdd);
bool Remove (const std::string &toRemove);
bool InList(const std::string &toSearch);
void PrintList() const;
private:
struct Node
{
std::string value;
Node *next;
};
Node *m_start;
};
#endif
// Answer to question :
// I indeed use a pointer to the next element.
// We couldn't have used a reference instead of a pointer because a reference
// needs to be initialized from the beginning.

MyLinkedList.cpp

#include <iostream>
#include "MyLinkedList.h"
// The constructor of the list.
MyLinkedList::MyLinkedList()
{
m_start = NULL;
}
// The destructor of the list.
MyLinkedList::~MyLinkedList()
{
while (m_start != NULL)
{
Node *p = m_start->next;
delete m_start;
m_start = p;
}
}
// Add an element to the end of the list and sets his next pointer to NULL.
void MyLinkedList::Add(const std::string &toAdd)
{
// If the element is in the list, won't do a thing.
if (InList(toAdd))
return;
// Reserve space for Node.
Node *temp = new Node;
// Assigns a copy of the toAdd string to the value of the node.
temp->value = std::string(toAdd);
temp->next = NULL;
// The list was empty.
if (m_start == NULL)
m_start = temp;
// There were already elements in the list, need to iterate till the end.
else
{
Node *temp2 = m_start;
while (temp2->next != NULL)
{
temp2 = temp2->next;
}
temp2->next = temp;
}
}
// Remove an element from the list and returns true if the operation succeeded
// (the element existed).
// The tricky part of the implementation is to always keep a pointer to the
// precedent element in order to set his next to the currentNode's next;
bool MyLinkedList::Remove (const std::string &toRemove)
{
// The element is not in the list.
if (!InList(toRemove))
return false;
Node *currentPointer, *previousPointer;
// For the first node there is not any pointer.
previousPointer = NULL;
// Visit each node, keeping a reference to the previous string each time.
for (currentPointer = m_start; currentPointer != NULL;
previousPointer = currentPointer,
currentPointer = currentPointer->next)
{
// We found the element to remove.
if (currentPointer->value == toRemove)
{
if (previousPointer == NULL)
{
// We fix the beginning pointer.
m_start = currentPointer->next;
}
else
{
// Fix previous node's next to skip the element deleted.
previousPointer->next = currentPointer->next;
}
delete currentPointer;
return true;
}
}
}
// Returns true if the given element is in the list, else false.
bool MyLinkedList::InList(const std::string &toSearch)
{
Node *current = m_start;
while (current != NULL)
{
if (current->value == toSearch)
return true;
current = current->next;
}
return false;
}
// Print the Linked List.
// 2 possibilities : 1/ Empty list: print Empty.
// 2/ List not empty: print every elements (only one
// per line).
void MyLinkedList::PrintList() const
{
Node *temp = m_start;
// The list is empty.
if (temp == NULL)
{
std::cout << "Empty\n";
return;
}
do
{
std::cout << temp->value << std::endl;
temp = temp->next;
}
while (temp!= NULL);
}

cyberjoac

20 septembre 2007 à 13:10:53

neuneutrinos

20 septembre 2007 à 13:30:38

ca à l'air pas mal mais tu aurais pu te simplifier la vie avec

using namespace std;

met le au debut apres les includes
(cela te permettera d'enlever les "std::")

MatteX

20 septembre 2007 à 16:10:55

Je trouve que c'est très bien fait,

passage de paramètre par références constantes c'est bon!

Pour améliorer l'ajout tu peux créer un pointeur m_end qui conserverai la position du dernier élément et ainsi tu pourrais ajouter ton nouvel élément immédiatement après celui-ci.

Pour la forme j'aurai créer une fonction empty() qui vérifierai si ma chaine contient au moins un élément.

20 septembre 2007 à 22:27:27

Merci beaucoup pour tes conseils je corrige ca de suite

lmghs

20 septembre 2007 à 22:41:08

Côté encapsulation, il n'y a pas grand chose de fait.
Tu peux déléguer une bonne partie de ton code aux noeuds. A commencer par un constructeur d'initialisation qui t'évitera d'accéder directement aux champs internes de ton noeud.

En gros, des découpages (/raffinements) ont été oubliés :
- construction des noeuds (donc)
- recherche d'un noeud

Pingouin chauffé

23 septembre 2007 à 15:33:01

Citation : neuneutrinos

tu aurais pu te simplifier la vie en utilisant using namespace std;

Encombrer l'espace de nom global est généralement une mauvaise idée. On fait ça pour simplifier des codes d'exemple, mais en règle générale c'est très déconseillé.

Debutant en C++

MyLinkedList.h

MyLinkedList.cpp

Besoin de conseils

Supprimer

Modérer

Signaler le message aux modérateurs