Returning a data structure

Discussion in 'C++' started by Hank Stalica, Jun 9, 2007.

  1. Hank Stalica

    Hank Stalica Guest

    So I've made a linked list class.

    I have a function that initiates this class, opens a file, parses it,
    and then inserts nodes into this temporary class.

    What I would like to do is then have it return that temporary class so I
    can assign it to a permanent class in the main program.

    I've written an assignment operator.

    So something like this is what I want to do:

    MyList loadMyList(string file)
    {
    MyList temp;

    //Do stuff to load temp.

    return temp;
    }

    in main:

    MyList list;
    list = loadMyList(fileName);

    I suspect that my datatype MyList is wrong...maybe it should be MyList*
    or MyList&?

    Best Regards,
    --Hank Stalica
    Hank Stalica, Jun 9, 2007
    #1
    1. Advertising

  2. Hank Stalica wrote:
    > So I've made a linked list class.


    std::list is not good enough for you ?

    >
    > I have a function that initiates this class, opens a file, parses it,
    > and then inserts nodes into this temporary class.


    Many elements in this list ? If you have a few thousand elements, it
    can take a while to copy the linked list every time.

    >
    > What I would like to do is then have it return that temporary class so I
    > can assign it to a permanent class in the main program.


    What does your linked list copy constructor do ?

    >
    > I've written an assignment operator.


    Did you make a copy constructor as well ?

    >
    > So something like this is what I want to do:
    >
    > MyList loadMyList(string file)


    Sure you don't mean:
    MyList loadMyList(const std::string & file)

    > {
    > MyList temp;
    >
    > //Do stuff to load temp.
    >
    > return temp;
    > }
    >
    > in main:
    >
    > MyList list;
    > list = loadMyList(fileName);
    >
    > I suspect that my datatype MyList is wrong...maybe it should be MyList*
    > or MyList&?


    It would work fine if MyList was a std::list.

    An alternative would be:

    MyList loadMyList(string file);
    ....
    MyList & list = loadMyList(fileName);

    This would eliminate the assignment operator. If you're lucky, you
    compiler will also do an RVO.

    Now, if large numbers of elements in you list, copying the list can get
    very expensive. It's usually a bad idea to perform memory intensive
    copying of elements if you can avoid it.

    My suggestions are as such:

    a) nuke MyList and use std::list
    b) use boost::shared_ptr and make a single copy of your list and avoid
    making deep copies. Alternatively, keep MyList, inherit from std::list
    and make it reference counted and use an Austria reference counted smart
    pointer. (shameless plug).

    I'll leave it at that at the moment. If you perhaps need more
    suggestions just ask.
    Gianni Mariani, Jun 9, 2007
    #2
    1. Advertising

  3. Hank Stalica

    Rolf Magnus Guest

    Hank Stalica wrote:

    > So I've made a linked list class.
    >
    > I have a function that initiates this class, opens a file, parses it,
    > and then inserts nodes into this temporary class.
    >
    > What I would like to do is then have it return that temporary class so I
    > can assign it to a permanent class in the main program.


    When you say 'class', you seem to mean 'object'.

    > I've written an assignment operator.
    >
    > So something like this is what I want to do:
    >
    > MyList loadMyList(string file)
    > {
    > MyList temp;
    >
    > //Do stuff to load temp.
    >
    > return temp;
    > }
    >
    > in main:
    >
    > MyList list;
    > list = loadMyList(fileName);
    >
    > I suspect that my datatype MyList is wrong...maybe it should be MyList*
    > or MyList&?


    What makes you believe that?
    Rolf Magnus, Jun 9, 2007
    #3
  4. "Hank Stalica" <> wrote in message news:...
    > So I've made a linked list class.
    >
    > I have a function that initiates this class, opens a file, parses it,
    > and then inserts nodes into this temporary class.
    >
    > What I would like to do is then have it return that temporary class so I
    > can assign it to a permanent class in the main program.
    >
    > I've written an assignment operator.
    >
    > So something like this is what I want to do:
    >
    > MyList loadMyList(string file)
    > {
    > MyList temp;
    >
    > //Do stuff to load temp.
    >
    > return temp;
    > }


    Ewwww. That won't work. It returns a temporary, so behavior is undefined.

    Use a parameterized constructor instead:

    class MyList
    {
    public:

    MyList(std::string file)
    {
    do stuff here to load records from file to list
    }

    (other public methods here, maybe)

    private:

    (internal data representation)
    }

    > in main:
    >
    > MyList list;
    > list = loadMyList(fileName);


    Ewwww, yuck. No, no, no. Soooooo many things wrong with that: returning
    temporaries, unneeded copying and assigning, etc., etc., etc..

    Do this instead:

    #include <iostream>
    #include <string>
    int main(int Beren, char * Luthien[])
    {
    std::string FileName(Luthien[1]); // get file name from cmmd-line arg.
    MyList list (FileName); // construct list from file
    ...
    return 0;
    }


    > I suspect that my datatype MyList is wrong...maybe it should be MyList*
    > or MyList&?


    Nope. That part you got right. But you need to learn about constructors.

    I'm assuming, also, that you're just implimenting lists for programming
    practice, or for a class. But for "real" code, use std::list instead.
    Then instead of MyList, you could use std::list<MyType> where "MyType"
    could be just about any kind of copy-constructable type imaginable.

    Something like this, perhaps (untested, incomplete; do the rest yourself):

    #include <iostream>
    #include <string>
    #include <list>

    struct EmployeeRecord
    {
    std::string first_name;
    std::string last_name;
    double wage_rate;
    }

    void
    BuildList
    (
    std::list<EmployeeRecord> const & EmpList,
    std::string const & FileName
    )
    {
    EmployeeRecord TempRec;

    Do stuff here to load employee records from file to EmpList.
    For each record in file, create a temporary EmployeeRecord object,
    then push a copy of the temporary object to the back of the list.

    Mixed C++ code and pseudocode:

    (open the file)
    while (1)
    {
    (try to get a record)
    if (EOF) break;
    (Load the record into TempRec)
    EmpList.push_back(TempRec);
    }
    return;
    }

    int main(int Beren, char * Luthien[])
    {
    // Get name of employee-records file from cmmd-line argument:
    std::string FileName(Luthien[1]);

    // Make a list:
    std::list<EmployeeRecord> EmpList;

    // Dump employee records from file into list:
    BuildList (EmpList, FileName);

    // Do other necessary stuff:
    ...

    return 0;
    }


    --
    Cheers,
    Robbie Hatley
    lone wolf aatt well dott com
    triple-dubya dott Tustin Free Zone dott org
    Robbie Hatley, Jun 9, 2007
    #4
  5. Robbie Hatley wrote:
    ....
    >> MyList loadMyList(string file)
    >> {
    >> MyList temp;
    >>
    >> //Do stuff to load temp.
    >>
    >> return temp;
    >> }

    >
    > Ewwww. That won't work. It returns a temporary, so behavior is undefined.


    It returns a copy of a temporary - works perfectly fine if the copy
    constructor works properly. Also, there is a an optimization called RVO
    (return value optimization) that on some compilers will eliminate the
    copy altogether, I don't know if it will work here tho).

    ....
    Gianni Mariani, Jun 9, 2007
    #5
  6. Hank Stalica

    Rolf Magnus Guest

    Robbie Hatley wrote:

    >> So something like this is what I want to do:
    >>
    >> MyList loadMyList(string file)
    >> {
    >> MyList temp;
    >>
    >> //Do stuff to load temp.
    >>
    >> return temp;
    >> }

    >
    > Ewwww. That won't work. It returns a temporary, so behavior is undefined.


    It returns a local variable by value, which is fine. You must be mixing that
    up with returning a reference to it.

    > Use a parameterized constructor instead:


    That would make MyList depend on the file loading functionality, which might
    not be desirable.

    > class MyList
    > {
    > public:
    >
    > MyList(std::string file)
    > {
    > do stuff here to load records from file to list
    > }
    >
    > (other public methods here, maybe)
    >
    > private:
    >
    > (internal data representation)
    > }


    ;

    >
    >> in main:
    >>
    >> MyList list;
    >> list = loadMyList(fileName);

    >
    > Ewwww, yuck. No, no, no. Soooooo many things wrong with that: returning
    > temporaries, unneeded copying and assigning, etc., etc., etc..


    You don't know anything about the implementation of MyList. The lists might
    share their internal data, which would make copying a very cheap operation.
    Also, many compilers implement return value optimization, which leaves only
    the assignment. Changing the above to:

    MyList list = loadMyList(fileName);

    would fix that, eliminating all MyList copy operations on any decent
    compiler.

    > Do this instead:
    >
    > #include <iostream>
    > #include <string>
    > int main(int Beren, char * Luthien[])
    > {
    > std::string FileName(Luthien[1]); // get file name from cmmd-line arg.


    Of course, you should always check if there is actually a Luthien[1].

    > MyList list (FileName); // construct list from file
    > ...
    > return 0;
    > }
    >
    >
    >> I suspect that my datatype MyList is wrong...maybe it should be MyList*
    >> or MyList&?

    >
    > Nope. That part you got right. But you need to learn about constructors.
    >
    > I'm assuming, also, that you're just implimenting lists for programming
    > practice, or for a class. But for "real" code, use std::list instead.


    Which doesn't have a constructor that takes the name of a file.
    Rolf Magnus, Jun 9, 2007
    #6
  7. "Gianni Mariani" <> wrote in message
    news:466a5ca1$0$22450$...
    > Hank Stalica wrote:


    > Many elements in this list ? If you have a few thousand elements, it can
    > take a while to copy the linked list every time.


    You wouldn't need to copy thousands of elements assuming there are thousands
    in there. If the list object is written correctly then one would just copy
    the head pointer because you have actually loaded the data into list nodes
    already using the temporary list (This 'list' object contains a pointer to
    the head list node, maybe a pointer to the tail node aswell if you are
    implementing a doubly linked list and functions to operate on the list)

    The other way to approach such a problem could be

    bool LoadMyList(std::string file, MyList& myList)
    {
    // Do whatever needs to be done to get contents of file into myList

    return FAIL/SUCCESS;
    }

    int main(int argc, char** argv)
    {
    MyList myList;

    if (LoadMyList("listdata.dat", myList) == FAIL)
    {
    // Handle the error
    return 1;
    }

    return 0;
    }

    But give std::list a look at because it is written quite well and it should
    meet your purpose.

    Adam Hamilton
    Adam Hamilton, Jun 9, 2007
    #7
  8. Hank Stalica

    Hank Stalica Guest

    Adam Hamilton wrote:
    >
    > "Gianni Mariani" <> wrote in message
    > news:466a5ca1$0$22450$...
    >> Hank Stalica wrote:

    >
    >> Many elements in this list ? If you have a few thousand elements, it
    >> can take a while to copy the linked list every time.

    >
    > You wouldn't need to copy thousands of elements assuming there are
    > thousands in there. If the list object is written correctly then one
    > would just copy the head pointer because you have actually loaded the
    > data into list nodes already using the temporary list (This 'list'
    > object contains a pointer to the head list node, maybe a pointer to the
    > tail node aswell if you are implementing a doubly linked list and
    > functions to operate on the list)
    >
    > The other way to approach such a problem could be
    >
    > bool LoadMyList(std::string file, MyList& myList)
    > {
    > // Do whatever needs to be done to get contents of file into myList
    >
    > return FAIL/SUCCESS;
    > }
    >
    > int main(int argc, char** argv)
    > {
    > MyList myList;
    >
    > if (LoadMyList("listdata.dat", myList) == FAIL)
    > {
    > // Handle the error
    > return 1;
    > }
    >
    > return 0;
    > }
    >
    > But give std::list a look at because it is written quite well and it
    > should meet your purpose.
    >
    > Adam Hamilton
    >
    >


    Thanks for the response everyone.

    I have actually used your example of passing the permanent list as you
    have done here:

    > bool LoadMyList(std::string file, MyList& myList)


    with another function I wrote and it worked. I didn't use std::list
    because I'm kind of writing this program as an academic exercise to get
    review how lists, stacks, queues, etc work.

    I was just trying to return the list to another list in a different way.
    My implementation of MyList is using a singly-linked list with no tail
    pointers.

    Is it better form to use your implementation where you return a bool
    instead of a MyList?

    Best Regards,

    --Hank Stalica
    Hank Stalica, Jun 10, 2007
    #8
  9. Hank Stalica

    BobR Guest

    Hank Stalica wrote in message...
    >
    > Is it better form to use your implementation where you return a bool
    > instead of a MyList?


    When you were returning a MyList, how did you indicate/handle errors like
    failure to load the file?

    // ref: bool LoadMyList( std::string const &file, MyList &myList);

    if( not LoadMyList(file, myList) ){ /* handle error */ }

    Look at that line, then decide.

    --
    Bob R
    POVrookie
    BobR, Jun 10, 2007
    #9
  10. Hank Stalica

    Rolf Magnus Guest

    BobR wrote:

    >
    > Hank Stalica wrote in message...
    >>
    >> Is it better form to use your implementation where you return a bool
    >> instead of a MyList?

    >
    > When you were returning a MyList, how did you indicate/handle errors like
    > failure to load the file?


    One possible way would be an exception.
    Rolf Magnus, Jun 10, 2007
    #10
    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. kj.kjn

    returning a structure using SWIG

    kj.kjn, Jul 2, 2003, in forum: Python
    Replies:
    0
    Views:
    297
    kj.kjn
    Jul 2, 2003
  2. Replies:
    11
    Views:
    651
    Christos Georgiou
    May 2, 2006
  3. ctj951
    Replies:
    4
    Views:
    304
    Leandro Melo
    Nov 14, 2008
  4. ctj951
    Replies:
    4
    Views:
    275
    Keith Thompson
    Nov 14, 2008
  5. A
    Replies:
    27
    Views:
    1,580
    Jorgen Grahn
    Apr 17, 2011
Loading...

Share This Page