C++ File reading from a list

Question

I am trying to parse the data from a file with certain structure. this data will be written into a double linked list (Next and prev) i have already made the function to read the data from the file and put into a temporal list, but when i try to pass it to the final list, somehow the data is missing. I think this is because of a bad linking between the fields of the list, but i cannot find the exact linking problem, the code is this:

void LeerArchivo (Persona **p, string doc){
    int n = 0;
    Persona *Aux = *p;
    Persona *t = new Persona;
    Fecha *f = new Fecha;
    Vehiculo *v = new Vehiculo;
    ifstream Archivo(doc.c_str()); // Abro el archivo en modo lectura

    if (Archivo)
    {
        while (!(Archivo.eof())){ // Mientras archivo no llegue al final del documento (end of file) ejecuta las instrucciones
            n = 0;
            Archivo >> t->Cedula; // Paso la primera linea de archivo al campo cedula del primer registro persona
            Archivo >> t->Nombre;
            Archivo >> t->Apellido;
            Archivo >> f->Dia;
            Archivo >> f->Mes;
            Archivo >> f->Annio;
            t->Nacimiento = f;
            Archivo >> n; // Esto sera un integer 1 o 0 contenido en el archivo que indica si la persona tiene carro o no
            if (n){
                Archivo >> v->Placa;
                Archivo >> v->Marca;
                Archivo >> v->Modelo;
                Archivo >> v->Color;
                Archivo >> v->Annio;
                v->Propietario = t;
                t->Carro = v;
            }
            else
                t->Carro = NULL;            

            if(Aux){
                t->Siguiente = NULL;
                (Aux)->Siguiente = t;
                t->Anterior = Aux;
                Aux = t;
                Aux = (Aux)->Siguiente;
            }
            else{
                Aux = t;
                t->Siguiente = NULL;
                t->Anterior = NULL;
            };
        };
    }
    else
    {
        cout << "El archivo " << doc << " no existe" << endl;
    }
};

When the parse is finished, the only data left in the list is the last register read from the file, so i am sure the data is being overwritten instead of being linked.

In the linking part i have also tried this pieces of code

 if(Aux){
    (*p)->Siguiente = NULL;
    (*p)->Siguiente = t;
    t->Anterior = *p;
    *p = t;
    *p = (*p)->Siguiente;
}
else{
    *p = t;
    t->Siguiente = NULL;
    t->Anterior = NULL;
};

And this other

if(Aux){
    (*p)->Siguiente = NULL;
    (*p)->Siguiente = t;
    t->Anterior = *p;
    *p = t;
}
else{
    *p = t;
    t->Siguiente = NULL;
    t->Anterior = NULL;
};

    *p = (*p)->Siguiente;

I will appreciate any suggestions about the code, thank you :D

Where in your code are you trying to add the new `Persona`, `Fecha` and `Vehiculo` to the list? I can't even see a list. Further, you can't check for `eof` before reading from `Archivo` - how can the compiler know if you'll hit `eof` when it doesn't know what you'll try to read yet? `eof` is set as you read, and you much check for it then, as in: `if (Archivo >> t->Cedula >> t->Nombre >> t->Appellido ... && (n == 0 || Archivo >> v->Placa ...) ...success... else ...eof or failure?...` — Tony Delroy, May 22 '14 at 02:32
The eof check works well, because the last register parsed remains stored, the lists Fecha and Vehiculo are hanging from Persona with a pointer, so i only need to add a new Persona, and that is when the code fails, every new Persona readed overwrite the last, so i think the problem is with the linking, because it keeps writing the data in the same field instead of passing to the next one. — nastrand, May 22 '14 at 02:41
your `eof` test will fail on an empty file, and if the last thing read for the final record - which could be `n` or `Annio` - is followed by any whitespace (even a newline). It also silently continues after being unable to parse data. You might want to do some background reading on the issue, say [here](http://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-considered-wrong). Separately, why don't you try using a `std::list` instead of all that pointer hackery? - you're more likely to get it right. — Tony Delroy, May 22 '14 at 02:52
I will check the eof issue, thanks about that, about the list thig, some university teacher sucks... mine asks to handle list this way. — nastrand, May 22 '14 at 02:57
Anyway, `Siguiente` and `Anterior` are your next/previous link pointers, so `Aux = t` will only work if you make `Aux` a `Persona*&` but then `Aux = (Aux)->Siguiente` wouldn't do what you want, so you should just use `*p = t;` instead.... This is the sort of problem where patiently using a debugger should show you whether you're changing the data you intended. — Tony Delroy, May 22 '14 at 03:03
I've tried replacing all the "Aux" calls with *p calls, in fact, that is the first version of the code, but doesnt work to, so, just for testing i try using an Aux var as pointer to store the parameter, but the problem still there, i will update the post with certain codes i've already tried. Thanks :D — nastrand, May 22 '14 at 03:21

Tony Delroy · Accepted Answer · 2014-05-22T14:09:07.947

So let's work through your latest attempt:

if(Aux)
    ...
else{
    *p = t;
    t->Siguiente = NULL;
    t->Anterior = NULL;
};

For the Aux == nullptr case, that looks ok. You should end up with:

p ---> *p = t ---> [ Siguiente nullptr
                     Anterior  nullptr ]

Then, for the second and subsequent records...

if(Aux){
    (*p)->Siguiente = NULL;
    (*p)->Siguiente = t;
    t->Anterior = *p;
    *p = t;
    *p = (*p)->Siguiente;
}
else ...

Aux is still a nullptr because you never updated it after adding the first record, so it never makes it into this code. Instead, it replaces the first element in the list with the just-read record. To fix this, you could add...

Aux = *p;

...to your else code, then subsequent records will reach:

    (*p)->Siguiente = NULL;
    (*p)->Siguiente = t;
    t->Anterior = *p;
    *p = t;
    *p = (*p)->Siguiente;

You're setting Siguiente twice... that can't be right! Going back to our data, if there's only one record so far then we start with the data arrangement from above, and our new *t record:

p ---> *p ---> [ Siguiente nullptr          t ---> [ Siguiente unset
                 Anterior  nullptr ]                 Anterior  unset ]

(*p)->Siguiente = t; changes the Siguiente pointer on the left to address t, the new record - sounds ok:

p ---> *p ---> [ Siguiente ---------------- t ---> [ Siguiente unset
                 Anterior  nullptr ]                 Anterior  unset ]

Then t->Anterior = *p;:

p ---> *p ---> [ Siguiente ---------------- t ---> [ Siguiente unset
                 Anterior  nullptr ]                 Anterior  +     ]
                 ^                                             |
                 |                                             |
                 +---------------------------------------------+

That's promising. But then you have this code...

    *p = t;

...which makes *p point off to the new record. You end up with this:

p ---> *p ------------------------------------------+
                                                    |
                                                    v
               [ Siguiente ---------------- t ---> [ Siguiente unset
                 Anterior  nullptr ]                 Anterior  +     ]
                 ^                                             |
                 |                                             |
                 +---------------------------------------------+

Then *p = (*p)->Siguiente;, and you've unlinked your old and new nodes and left *p with a copy of an uninitialised pointer:

p ---> *p = unset

               [ Siguiente ---------------- t ---> [ Siguiente unset
                 Anterior  nullptr ]                 Anterior  +     ]
                 ^                                             |
                 |                                             |
                 +---------------------------------------------+

If you work through what you're doing like this - on paper - you should be able to correct your code.

That's a +1 for representation alone, prior to even reading question or answer...:) — gnometorule, May 22 '14 at 03:56
@gnometorule :D thanks mate! Took longer than I'd like to admit ;-). — Tony Delroy, May 22 '14 at 04:12
Thanks for the awesome explanation, bottom line i need to let this two lines (*p)->Siguiente = t; and t->Anterior = *p; for the linking, but for some reason this keeps overwriting the data, this doesnt make sense at all, it supposed to insert the new field on the head of the list and make it point, but the problem its all the list point to the "t" variable, because im making the insertion like in a recursive function, your explanation make me understand the problem, now i will try to fix it. Thanks! — nastrand, May 22 '14 at 13:54
@user2759595 having another look, I think I got the last two bits wrong - `*p = t;` onwards - I've updated the explanation to show what they're doing. Good luck with it - it's not impossible, but it takes patience and it really is best to work through it in paper step by step! — Tony Delroy, May 22 '14 at 14:10
@TonyD after checking my code a bit, i will change some functions to use an add functions previously defined to receive an input by keyboard, so i will make the data adding recursive to avoid the problems with the cycle adding, to avoid the cycle pointer that happen when p points to t, in the recursive the T function will be deleted on every iteration, so it must work well. Again, thanks for your time! — nastrand, May 22 '14 at 14:20

C++ File reading from a list

1 Answers1