Like Tree1Likes
  • 1 Post By laserlight

2d array pointers code review

This is a discussion on 2d array pointers code review within the C++ Programming forums, part of the General Programming Boards category; I'm currently working my way through ' Jumping into C++ ', having never really sat down with C++ before. I'm ...

  1. #1
    Registered User
    Join Date
    Jul 2012
    Posts
    10

    2d array pointers code review

    I'm currently working my way through 'Jumping into C++', having never really sat down with C++ before. I'm currently getting to pointers and definitely need a good bit of practice there - I don't really feel comfortable using them yet as well as dynamically allocating memory.

    As far as I can tell the following code runs and does what I want it to do (dynamically allocate memory for a 2d array, fill the array, print it out, and delete it, all done by passing the pointer pointer of the array to functions as arguments) but I just wanted someone with actual C++ history to take a look at it and advise me if I'm doing anything weird: https://gist.github.com/39f795b6c56e838bada2

    And what the output should be:
    Code:
     empty array, cols: 2, rows: 3 
    |0    |0    | 
    |0    |0    | 
    |0    |0    | 
    
     multiplication table: 
    |1    |2    | 
    |2    |4    | 
    |3    |6    | 
    
     -deleting array- 
    
     empty array, cols: 5, rows: 7 
    |0    |0    |0    |0    |0    | 
    |0    |0    |0    |0    |0    | 
    |0    |0    |0    |0    |0    | 
    |0    |0    |0    |0    |0    | 
    |0    |0    |0    |0    |0    | 
    |0    |0    |0    |0    |0    | 
    |0    |0    |0    |0    |0    | 
    
     multiplication table: 
    |1    |2    |3    |4    |5    | 
    |2    |4    |6    |8    |10   | 
    |3    |6    |9    |12   |15   | 
    |4    |8    |12   |16   |20   | 
    |5    |10   |15   |20   |25   | 
    |6    |12   |18   |24   |30   | 
    |7    |14   |21   |28   |35   |

    Basically any advice or suggestions are welcome.

    For example, I have read before that generally you'd want to wrap your array in a class, but I haven't gotten to classes yet in the book

  2. #2
    Registered User
    Join Date
    Jun 2005
    Posts
    6,658
    Look at your function pparray_delete(). It is deleting a single pointer, multiple times. Undefined behaviour.

    It would be better to write your code using a std::vector<std::vector<int> >. If you do that, you wouldn't even need to use operators new and delete at all (except to the extent they are used by vector's allocators, which you shouldn't even need to worry about). You would then be able to eliminate the pparray_create() and pparray_delete functions and greatly simplify the implementation of pparray_multiplication_table() and pparray_print() functions.

    The style of your code is essentially idiomatic C, albeit using some C++ language features (eg using operator new instead of malloc(), operator delete instead of free()). That is either good or bad, depending on your perspective. But, the approach is some years behind modern idiomatic C++.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Sunshine, and read this, this, and this before posting again.

  3. #3
    Registered User
    Join Date
    Jul 2012
    Posts
    10
    Ah crap, that should have been an 'i' there assuming it was the following then: https://gist.github.com/b650708f132b980cab24 does it actually do what I want it to, free up the array?

    I haven't gotten to vectors yet, but I'll keep that in mind though in the future.

    I'm a total C++ noob, so forgive me if I have no idea what 'idiomatic C' means (I've been 'programming' C++ for less than a week at this point)

  4. #4
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    23,021
    You are still invoking undefined behavior. For every new[], there must be a delete[]. For every new, there must be a delete. You must not mix these types.
    What grumpy is referring to is that you are basically doing this the "C way." That is, this is how one would do it in C, and since C++ is backwards compatible with C, it is possible to do it in C++ this way, too, but it is considered bad practices because it's easy to get wrong, and there are better ways (eg std::vector).
    If you are merely doing this to get a better feel for pointers, then that is fine. But don't consider using this in "production" code, ie any non-toy project that intend to use for some practical purposes.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  5. #5
    Registered User
    Join Date
    Jul 2012
    Posts
    10
    Quote Originally Posted by Elysia View Post
    You are still invoking undefined behavior. For every new[], there must be a delete[]. For every new, there must be a delete. You must not mix these types.
    I'm not sure I understand what I still need to do. Do you mean I need to use "delete[] pparray;" instead of "delete pparray;" on line 26? Could you write a version of pparray_delete() that does it correctly? As I've said a few times now, I really don't have much of an idea of what I'm doing

    Quote Originally Posted by Elysia View Post
    If you are merely doing this to get a better feel for pointers, then that is fine. But don't consider using this in "production" code, ie any non-toy project that intend to use for some practical purposes.
    Hahaha, yeah, production code after less than a week of experience, that would be quite something at this rate I doubt I'd trust myself with anything like that for another year.
    Last edited by Johannes; 07-20-2012 at 12:12 PM.

  6. #6
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    23,021
    How did you allocate myarray? With new[].
    How you you delete myarray? With delete.
    Undefined behavior.
    Get my point?
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  7. #7
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    22,303
    Basically, change this:
    Code:
    delete pparray;
    to:
    Code:
    delete[] pparray;
    Johannes likes this.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  8. #8
    Registered User
    Join Date
    Jul 2012
    Posts
    10
    Quote Originally Posted by laserlight View Post
    Basically, change this:
    Code:
    delete pparray;
    to:
    Code:
    delete[] pparray;
    Thank you! Feedback that is actually useful to someone that is very much a beginner =)

  9. #9
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    23,021
    Actually, I was going to let you figure it out for yourself, so that you can grasp exactly what you are doing and the difference between new and new[] (and delete and delete[]).
    But I guess this also goes to show how error prone this is, and precisely why we do not recommend it.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  10. #10
    Registered User
    Join Date
    Jun 2005
    Posts
    6,658
    Quote Originally Posted by Johannes View Post
    I'm a total C++ noob, so forgive me if I have no idea what 'idiomatic C' means (I've been 'programming' C++ for less than a week at this point)
    Idiomatic is an english word. It means "natural practice or common practice in the language".

    C and C++, despite being related, are usually used in different ways. You are doing things in the way of C, not C++.

    The difference is akin to the way native speakers of a natural language (english, french, ....) express themselves differently from non-native speakers or, in your case, in ways that natives speakers of other languages would.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Sunshine, and read this, this, and this before posting again.

  11. #11
    Registered User whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    7,762
    Quote Originally Posted by Elysia View Post
    But I guess this also goes to show how error prone this is, and precisely why we do not recommend it.
    Don't presume to speak for other people.

  12. #12
    Registered User
    Join Date
    Jun 2005
    Posts
    6,658
    Quote Originally Posted by whiteflags View Post
    Don't presume to speak for other people.
    There are plenty of other people who make exactly the observation and recommendation that Elysia did. I am among them. More noteworthy people than I who do so include Bjarne Stroustrup, Scott Meyers, and Alex Stepanov.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Sunshine, and read this, this, and this before posting again.

  13. #13
    Registered User whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    7,762
    I dunno, whenever I say something about a particular group, I always get in trouble so I just got away from doing that in general. In this case, the only thing making the observation does is insist that the OP not use what he only just learned.
    Last edited by whiteflags; 07-20-2012 at 06:49 PM.

  14. #14
    Registered User
    Join Date
    Jun 2005
    Posts
    6,658
    Maybe so. However, it's a subject of debate whether he should have been taught about pointers in the first place. The thing is, in C++, a beginner who has learned about the standard library can do a lot of things, and do them safely, even with knowing nothing about pointers. However, a beginner who has learned about pointers can do stuff, but the techniques are error prone (eg avoiding molesting a pointer) and will eventually need to learn about the standard library anyway.

    No question that, in the long run, someone learning C++ needs to know both about the standard library, and about pointers (as well as dynamic memory allocation). But I'm of the view that it is better to teach safe approaches (eg using the standard library) before teaching unsafe approaches (eg pointers, explicit dynamic memory allocation, and pointer arithmetic). When teaching C++, most things involving pointers can be safely left until later (for example, as an advanced topic) without forcing the student to be unproductive.

    Of course, that flies in the traditional approach of teaching C - which amounts to teaching unsafe techniques and forcing beginners to experience their own personal baptism by fire by which they learn to avoid the pitfalls. It also flies in the face of folks who insist on teaching C++ in the same way as C (eg as an extended C). But the traditional teaching approach for C (and often for C++) is akin to a carpentry teacher turning on a circular saw and telling students to use it to cut wood, with no other preparation. There are good reasons beginners to carpentry aren't taught that way. And there are also good reasons to suggest that beginners to C++ shouldn't be taught that way either. [There is also a case to suggest that beginners to C shouldn't be taught that way either, but C doesn't have as many safe and effective alternatives to pointers as does C++].
    Last edited by grumpy; 07-20-2012 at 07:24 PM.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Sunshine, and read this, this, and this before posting again.

  15. #15
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    23,021
    Quote Originally Posted by whiteflags View Post
    Don't presume to speak for other people.
    The is the general recommendation of many members of the board, hence I feel that the word "we," is appropriate here.
    It does, of course not, mean that I speak for everyone, but that I am implying that many, or most, board members feel the same (otherwise they would denounce that claim, obviously).
    That said, if you feel like you don't agree with that, feel free to exclude yourself from the definition of we. I'm sorry if you feel that way, though.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Code review
    By Elysia in forum C++ Programming
    Replies: 71
    Last Post: 05-13-2008, 10:42 PM
  2. review this code
    By KIBO in forum C Programming
    Replies: 12
    Last Post: 08-14-2007, 03:28 PM
  3. code review
    By hamsteroid in forum C Programming
    Replies: 6
    Last Post: 04-04-2007, 01:23 PM
  4. Code review please
    By Brighteyes in forum C Programming
    Replies: 9
    Last Post: 03-29-2003, 06:28 PM
  5. review code
    By absenta in forum C++ Programming
    Replies: 3
    Last Post: 04-09-2002, 03:13 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21