1

I am studying the following code but get confused on the building of the single linked list in function read_file(), I have marked the question at the function next to the code below:

here is the full code: https://github.com/mickyjm/c.cpp.address.book.project/tree/master/CPP

list header file list.h

#ifndef LLIST_H
#define LLIST_H
#include <string>
class llist {
private:
    record *start;
    std::string file_name;
    int read_file();
    int write_file();
    record* reverse_llist(record *);
    void delete_all_records();

public:
    llist();
    llist(std::string);
    ~llist();
    int add_record(std::string, std::string, int, std::string);
    int print_record(std::string);
    int modify_record(std::string, std::string, std::string);
    void print_all_records();
    int delete_record(std::string);
    void reverse_llist();
};
#endif

record header file record.h

#include <string>
#ifndef RECORD_H
#define RECORD_H
struct record {
    std::string name;
    std::string address;
    int birth_year;
    std::string phone_number;
    struct record* next;
};
#endif

list.cpp read_file function

int llist::read_file() {
    // read_file variables
    std::ifstream read_file(file_name.c_str());
    struct record *temp = NULL;
    struct record *index = NULL;
    struct record *previous = NULL;
    int file_char_length = 0;
    int record_count = 0;
    std::string dummy = "";

    if (!read_file.is_open()) {
        read_file.close();
        return -1;
    } // end if !read_file.is_open()
    read_file.seekg(0, read_file.end); // move read pointer to end of file
    file_char_length = read_file.tellg(); // return file pointer position
    if (file_char_length == 0) {
        read_file.close();
        return 0;
    } // end file_char_length == 0
    read_file.seekg(0, read_file.beg); // reset file pointer to beginning
    do { // do while !read_file.eof()
        // do while temporary variables
        std::string address = "";

        temp = new record;
        index = start;
        std::getline(read_file, temp->name);
        std::getline(read_file, temp->address, '$');
        read_file >> temp->birth_year;
        std::getline(read_file, dummy);
        std::getline(read_file, temp->phone_number);
        std::getline(read_file, dummy);
        ++record_count;
        while (index != NULL) {                <-- what's the purpose of this loop?
            previous = index;                      
            index = index->next;
        } // end while index != NULL
        if (previous == NULL) {             <-- why would the start pointer of the
            temp->next = start;                 list not at the start but after temp?
            start = temp;
        } else { // else if previous != NULL    
            previous->next = temp;           <-- what is the purpose of this loop?
            temp->next = index;
        } // end if previous == NULL                         
    } while (!read_file.eof()); // end do while
    read_file.close();
    return record_count; // read_file return - end of function
}
Nigel
  • 161
  • 12
  • 1
    Marginally related: [Why is iostream::eof inside a loop condition (i.e. `while (!stream.eof())`) considered wrong?](https://stackoverflow.com/questions/5605125/why-is-iostreameof-inside-a-loop-condition-i-e-while-stream-eof-cons) – user4581301 Mar 08 '22 at 05:56
  • Don't waste your time computing the size of the file. If you follow the advice given in the link above you won't need to. – user4581301 Mar 08 '22 at 05:57
  • `while (index != NULL)` Find the end of the list. – user4581301 Mar 08 '22 at 05:58
  • `if (previous == NULL)` tests for an empty list. It could have been `if (start)` and could have been used to eliminate the above loop if the list was empty. – user4581301 Mar 08 '22 at 06:01
  • Not really worth learning from this list implementation. It's confusing because it is poorly conceived. As you add items to the list, don't look k for the end every time because the end will be the last node you added. Just remember the last node, add directly to it ,and half the code here just goes away. – user4581301 Mar 08 '22 at 06:04
  • 1
    Generally, you have the author, so ask them. To them, I would also suggest codereview.stackexchange.com, there is a lot to improve. To you, that also means there are signs in this code of a limited understanding of C++, so don't take this as good example. Further, learn to use a debugger! There are video tutorials out there, which are probably the easiest way to start. You could then inspect the code in action and answer those questions yourself. – Ulrich Eckhardt Mar 08 '22 at 06:25
  • I have try send the author an email but haven't got any reply. thanks for the comments. – Nigel Mar 08 '22 at 07:32

1 Answers1

1

In order of comments asked.

  1. What's the purpose of this loop?
while (index != NULL) { 
    previous = index;                      
    index = index->next;
} // end while index != NULL

Answer: this loop is used to position previous on the last node in the list, so long as there is at least one node. There are multiple techniques to do this; this is one of the more common. As index races to the end of the loop previous stays one step behind.

  1. Why would the start pointer of the list not at the start but after temp?
if (previous == NULL) { 
    temp->next = start;                 
    start = temp;

Answer: fighting the grammar of that question, the usage of start here as the right-side of this assignment is pointless. It must be NULL or the prior loop would have loaded previous with a non-NULL value and this if-condition would have failed. The code could just as easily read: temp->next = nullptr; start = temp;

  1. what is the purpose of this loop?
} else { // else if previous != NULL    
    previous->next = temp;
    temp->next = index;
} // end if previous == NULL 

Answer: First, this isn't a loop. This is the else clause to the prior if, which means if this runs it is because previous was non-null. Similar to the odd usage of start in (2) above, the use of index here is pointless. If you look to the loop you asked about in (1) you clearly see that thing doesn't stop until index is NULL. Nothing between there and here changes that fact. Therefore, temp->next = nullptr; is equivalent to what is happening here.

Not sugarcoating it; this implementation is weak, and that is on a good day. Whomever it came from, consider it a collection of how things can be done, but by no means how they should be done.

WhozCraig
  • 65,258
  • 11
  • 75
  • 141