SImple list implementation

[ckroom]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Could anybody tell me if this code is right?

Thanks.


struct lista_alumnos
{

~ struct alumno datos;

~ struct lista_alumnos *siguiente;

};


void InsertaAlumno (struct lista_alumnos &alumnos,
~ const struct alumno &nuevo_alumno)
{

~ lista_alumnos *nuevo_nodo, *iterador;

~ nuevo_nodo = new struct lista_alumnos;

~ nuevo_nodo->datos = nuevo_alumno;


~ for( iterador = alumnos;
~ iterador != NULL;
~ iterador = iterador->siguiente );

~ iterador = nuevo_nodo;

}



- --
F. J. Yáñez (ckroom)

http://nazaries.net/~ckroom

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFBL0WBDhQpyJGbSuMRAtD7AJ9MHJdAjcfbKLvpGQ/I2A5/mUWWRACfX5iV
aFMwQvYvBO4ibq3kvEwAOxk=
=B7hu
-----END PGP SIGNATURE-----

 

[Victor Bazarov]

Could anybody tell me if this code is right?

Thanks.

Even after you remove superfluous 'struct' occurrences, define 'alumno',
it won't even compile. It contains logical errors, too.

struct lista_alumnos
{

~ struct alumno datos;

~ struct lista_alumnos *siguiente;

};


void InsertaAlumno (struct lista_alumnos &alumnos,
~ const struct alumno &nuevo_alumno)
{

~ lista_alumnos *nuevo_nodo, *iterador;

~ nuevo_nodo = new struct lista_alumnos;

nuevo_nodo = new lista_alumnos(); // parentheses force the members to 0

~ nuevo_nodo->datos = nuevo_alumno;


~ for( iterador = alumnos;

You cannot assign an object to a pointer. To make it compile you need to
do
for (iterador = &alumnos;

~ iterador != NULL;
~ iterador = iterador->siguiente );

~ iterador = nuevo_nodo;

In these two last statements you're scanning for the end of the list,
but you never remember to update the list itself after the loop has
found the end.

alumnos:
/ datos \ .---> / datos \ .---> ... / datos \
\ siguiente /---' \ siguiente /---' \ NULL / -->X

That's the desired structure. Now, imagine that you do have it correctly
laid out in memory. What happens when you search for the last element?
You iterate the list with the only variable 'iterador'. It gets the value
'NULL' at the last update (iterador = iterador->siguiente), and then what?
You make the _local_ variable (iterador) to point to the dynamic object
you just created. But the list (the last element of that list) has not
been updated.

Instead of having 'iterador' remember the 'siguiente' it found, make it
store the _previous_ element. How? Instead of testing the value of the
'iterador' pointer, test 'iterador->siguiente':

for (iterador = alumnos; iterador->siguiente != NULL;
iterador=iterador->siguiente);

Now, when the loop finishes, 'iterador' will point to the last element of
the list (the tail), and you do

iterador->siguiente = nuevo_nodo;

}

Victor

 

[Howard]

Could anybody tell me if this code is right?

Thanks.


struct lista_alumnos
{

~ struct alumno datos;

What are the ~ symbols? (Tab characters?)

You don't need the word struct there (or anywhere else, except where you
actually define the struct's).

~ struct lista_alumnos *siguiente;

Since you have dynamically-allocated data in this struct, you're probably
going to need three functions: a constructor (which initializes the
pointer), a destructor (which manages deleting the list pointed to by that
pointer), and an assignment (=) operator (which copies the contents from one
lista_alumnos to another, including allocating and copying the list
contents).

};


void InsertaAlumno (struct lista_alumnos &alumnos,
~ const struct alumno &nuevo_alumno)
{

~ lista_alumnos *nuevo_nodo, *iterador;

~ nuevo_nodo = new struct lista_alumnos;

~ nuevo_nodo->datos = nuevo_alumno;

Ok, you've made a new list structure (node), and copied the given alumno
object to it's alumno member. But you don't show the alumno structure
anywhere. If it has any dynamically-allocated members (pointers), then
you'll need to provide an assignment operator for it. If not, the default
one the member-wise copy that you'll get by default should work ok.

~ for( iterador = alumnos;
~ iterador != NULL;
~ iterador = iterador->siguiente );

Here, you went from the beginning of the given list to the end. Now what?
When this loop exists, your iterator (iterador) is NULL. That's the exit
condition of the loop. I'm guessing what you really want is to exit when
iterador->siguiente is NULL. That way, iterador will point to the last node
in the list. Then, you can assign that node's next (siguente) pointer to
point to the new node you just created, instead of doing whatever it is you
thought the line below was going to do.

(One problem with my suggestion that you have to work out: what if the list
is empty to begin with? You need to handle that case without going into the
loop in the first place. I'll leave that to you to handle.)

~ iterador = nuevo_nodo;

This is just useless. For one thing, terador is a local variable. Setting
it to point to the new node accomplishes nothing at all, because it will go
out of scoe at the end of the function. (See the comments above for what I
think you meant to do.)

}

-Howard