[RESOLVED] Help reviewing C++ Test
CodeGuru Home VC++ / MFC / C++ .NET / C# Visual Basic VB Forums Developer.com
Results 1 to 10 of 10

Thread: [RESOLVED] Help reviewing C++ Test

  1. #1
    Join Date
    Sep 2012
    Posts
    5

    [RESOLVED] Help reviewing C++ Test

    Hi There. This is my fist post. I'm an aspiring programmer looking for a position as a C++ programmer. I am currently an iOS developer as well as having experience with C# in Unity, but my C++ is not as strong as my skill with these other languages. I recently applied to a company for a mid level C++ programmer. I don't want to disclose the name because they asked my not to share the test publicly. The test they sent me was pretty complex and definitely more advanced than I expected so I did not get the position. I would really like to know what I did wrong, what I did right, and what I could of done better so I can learn from the experience and hopefully improve myself. The test was to complete a container class they has which was like a memory pool. I was passed in a char * and that was the only memory I could use. I thought I had it going in the right direction, but I definitely did not get it right because it would crash when I ran it and like I said, it was more advanced than I was used to.

    Iwas wondering if there were any C++ experts that wouldn't mind taking 5 or 10 minutes looking at my class and letting me know where I went wrong. I would really appreciate it and I really think if I could have my errors explained it would help to visualize why I did it wrong. The class is not too big, only about 200 lines.

    If any body is willing to help a fellow programmer out I would be very greatfull. I don't want to post any code here because I agreed that I wouldn't publicly share it, but if you are willing to help I will PM you the file.

    Thanks so much for anybody willing to take a look. You're really helping me out and helping me learn.

    Regards,

    John

  2. #2
    VictorN's Avatar
    VictorN is online now Super Moderator Power Poster
    Join Date
    Jan 2003
    Location
    Wallisellen (ZH), Switzerland
    Posts
    17,418

    Re: Help reviewing C++ Test

    What problems (exactly!) do you have with your class?
    if you "don't want to post any code here" than the probability to get a real help will be not high.
    Just post the parts you have problems with. Note however, that these code parts must be compilable!
    Victor Nijegorodov

  3. #3
    Join Date
    Sep 2012
    Posts
    5

    Re: Help reviewing C++ Test

    deleted
    Last edited by jingato; September 12th, 2012 at 09:57 PM.

  4. #4
    GCDEF is offline Elite Member Power Poster
    Join Date
    Nov 2003
    Posts
    12,107

    Re: Help reviewing C++ Test

    You said it crashed when you ran it. You didn't post enough code to actually run it. What did the debugger show you?

  5. #5
    Join Date
    Apr 1999
    Posts
    27,430

    Re: Help reviewing C++ Test

    Quote Originally Posted by jingato View Post
    Well, I was trying to only send it to the poeple who were interested, but I guess If I don't give the name of the company it was for it should be ok.
    1) Your class lacks a copy constructor, assignment operator, and destructor that would handle the dynamically allocated memory.

    2) The class has memory leaks since you didn't implement the destructor in CContainer. You also don't delete the CItem you attempt to remove in the Remove() function.

    Without 2), any utility the class may have becomes useless. Even if you had a perfectly running Add() function, you can't use such a class in a real program, since it would leak memory. Once you implement 2), you then run into the problem of item 1), which is the copying aspect.

    I think what may have cost you is the lack of attention to not leak memory (there isn't a single call to "delete" as far as I can tell). That is a bad sign and a major oversight for anyone applying for a job as a C++ programmer. As far as the test goes, anyone can make a logic mistake, but the lack of cleanup of memory is not a mistake in logic.

    As to the implementation of the destructor, if you implemented the destructor, then I could easily crash your code with a two or three line main program.
    Code:
    int main()
    {
        char pbuffer[] = "abc123";
        CContainer<int> c1(pbuffer, sizeof(pbuffer));
        CContainer<int> c2(pbuffer, sizeof(pbuffer));
        c1 = c2;
    }
    If the destructor were implemented, then the program above would have a memory leak and a double deletion error, possibly causing a crash on the return of main(). The way you avoid the crash is to implement the copy-assignment operations, or make these operations private functions and unimplemented.

    Since you didn't post any code showing your main() program (a class can't live just by itself, it needs a driver to test it), I'm showing how it is very easy to render the code you have now very breakable. To get a complete, bug free (or close to bug free) implementation, you need to address the two items above.

    Regards,

    Paul McKenzie
    Last edited by Paul McKenzie; September 6th, 2012 at 04:38 PM.

  6. #6
    Join Date
    Apr 1999
    Posts
    27,430

    Re: Help reviewing C++ Test

    Quote Originally Posted by jingato View Post
    The test they sent me was pretty complex and definitely more advanced than I expected so I did not get the position. I would really like to know what I did wrong, what I did right, and what I could of done better so I can learn from the experience and hopefully improve myself.
    Well, since the test seems to be using a linked list, you should have created a solid, working, linked list class, and forget temporarily about what you were going to use the linked list for (in this case a memory pool). Then you would have implemented the copy-assignment, destructor, etc., tested it, etc. before applying it to the larger program.

    If you did that, then the rest becomes simple -- you would then take the working linked list class, and just use it as a component into the memory pool program. The focus then shifts to the logic of the memory pool and not the logic of the linked list class. Instead, what you did is the classic misstep of trying to "inline" your data structure code into the guts of your main program. You don't know where to start debugging -- the linked list code, the memory pool itself, or some other aspect, as everything is intertwined.

    If you took the approach of creating the linked list class by itself, and then attempted to use it to create the memory pool application, then it would give the good impression to the ones looking at the code that you know how to encapsulate and break up a program into logical, working components, and then use the working components to create the larger program.

    Regards,

    Paul McKenzie

  7. #7
    Join Date
    Jun 2010
    Location
    Germany
    Posts
    2,585

    Re: Help reviewing C++ Test

    Although your advice to separate the implementation of the linked list from that of the pool-based container using it is quite valuable (as usual ), as I understand the assignment, using a linked list is the wrong approach to begin with. As the memory block to be used as the pool is passed in on container construction and then remains constant (except for its content, of course) and the payload objects (T) are of constant size, using a linked list wouldn't really gain much; mostly it would just increase complexity and waste some of the precious pool memory for the link pointers.

    I'd rather treat the memory pool as a plain array of payload objects, backed by a std::vector<bool> to keep track of which positions in the array actually contain valid constructed T instances. However, if the use of the vector is forbidden by the constraint "I was passed in a char * and that was the only memory I could use" (I'm unsure how strict the constraint actually is), I might alternatively treat the memory pool as an array of structures similar to the list node struct from post #3:

    Code:
    struct CItem
    {
      T data;  // Actual payload instance, not a pointer
      bool valid;
    };
    Another thing I'm uncertain about is the relation between the constraint that the payload objects must not be moved in memory once they have been constructed and the exact semantics of Remove() and operator[]().
    I was thrown out of college for cheating on the metaphysics exam; I looked into the soul of the boy sitting next to me.

    This is a snakeskin jacket! And for me it's a symbol of my individuality, and my belief... in personal freedom.

  8. #8
    Join Date
    Jul 2005
    Location
    Netherlands
    Posts
    2,013

    Re: Help reviewing C++ Test

    Quote Originally Posted by Eri523 View Post
    Another thing I'm uncertain about is the relation between the constraint that the payload objects must not be moved in memory once they have been constructed and the exact semantics of Remove() and operator[]().
    I think this mostly relates to the sort function. If you use a linked list you can sort the items without moving/swapping the data.
    Cheers, D Drmmr

    Please put [code][/code] tags around your code to preserve indentation and make it more readable.

    As long as man ascribes to himself what is merely a posibility, he will not work for the attainment of it. - P. D. Ouspensky

  9. #9
    Join Date
    Jul 2005
    Location
    Netherlands
    Posts
    2,013

    Re: Help reviewing C++ Test

    Quote Originally Posted by jingato View Post
    and this what I did.
    I think the first mistake you made is that you made your class too complex. You have a doubly linked list, keep track of the size, keep track of both used and unused items, ... There are just too many invariants that you can break. If I were given this assignment, I'd probably create a singly linked list and certainly wouldn't keep track of the size. Since there are no performance requirements stated in the assignment, it doesn't matter if your Count function runs in O(n).
    Quote Originally Posted by jingato View Post
    Code:
    // Struct to hold data and create linked list
    template<typename T>
    struct CItem
    {
    	T *data;
    	CItem *next;
    	CItem *prev;
    };
    Think about this: you use all the memory you are given to create links, but didn't allocate any instance of T. How are you going to add any T afterwards? You're already out of memory.

    Perhaps you should first try to implement your own linked list class.
    Last edited by D_Drmmr; September 7th, 2012 at 05:42 PM. Reason: fixed typo
    Cheers, D Drmmr

    Please put [code][/code] tags around your code to preserve indentation and make it more readable.

    As long as man ascribes to himself what is merely a posibility, he will not work for the attainment of it. - P. D. Ouspensky

  10. #10
    Join Date
    Jun 2010
    Location
    Germany
    Posts
    2,585

    Re: Help reviewing C++ Test

    Quote Originally Posted by D_Drmmr View Post
    I think this mostly relates to the sort function. If you use a linked list you can sort the items without moving/swapping the data.
    I already thought about the sort feature a bit but omitted that from post #7 for brevity. I considered implementing the sort by using an std::vector<int> to map from the item indices as seen by the client code to the actual indices of the items in the pool array.

    My primary concern about the semantics of Remove() and operator[]() was whether the exposed item indices were supposed to remain constant (like the item addresses), which would imply that removing items (except the last one) leads to invalid indices within the range fom 0 to Count() - 1 for which operator[]() probably should return nullptr or throw an exception. (And by the way that would make the semantics of Count() rather abstract.)

    However, and that was something I failed to consider before reading your post, once sorting gets introduced, constant item indices can't be maintained anymore anyway, rendering the "invalid indices" approach pretty senseless. So, properly anticipating the effect of sorting, that approach obviously is not reasonable anyway.
    I was thrown out of college for cheating on the metaphysics exam; I looked into the soul of the boy sitting next to me.

    This is a snakeskin jacket! And for me it's a symbol of my individuality, and my belief... in personal freedom.

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  


Windows Mobile Development Center


Click Here to Expand Forum to Full Width

This is a CodeGuru survey question.


Featured


HTML5 Development Center