SImple list implementation

Discussion in 'C++' started by ckroom, Aug 27, 2004.

  1. ckroom

    ckroom Guest

    -----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-----
     
    ckroom, Aug 27, 2004
    #1
    1. Advertising

  2. ckroom wrote:
    > 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
     
    Victor Bazarov, Aug 27, 2004
    #2
    1. Advertising

  3. ckroom

    Howard Guest

    "ckroom" <> wrote in message
    news:...

    > 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
     
    Howard, Aug 27, 2004
    #3
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Frank Gerlach
    Replies:
    0
    Views:
    588
    Frank Gerlach
    Nov 22, 2004
  2. dondraper
    Replies:
    2
    Views:
    727
    dondraper
    Jan 27, 2006
  3. Michael Tsang
    Replies:
    32
    Views:
    1,139
    Richard Bos
    Mar 1, 2010
  4. Michael Tsang
    Replies:
    54
    Views:
    1,219
    Phil Carmody
    Mar 30, 2010
  5. sanket
    Replies:
    7
    Views:
    1,045
    Tsung
    Nov 3, 2011
Loading...

Share This Page