1

I'm learning c++ and one of the questions I was asked to do was "Read the seats prices into a 2D dynamic array. The seat prices are stored in a text file ‘prices.txt’. User double pointer for the prices array." My code isn't printing out the text file and I'm not sure what I did wrong. Here is my code:

#include <fstream>
#include <iomanip>
using namespace std;

void fillarray(int**a, int rows, int cols)
{
  ifstream file;
  file.open("prices.txt");
  while(!file.eof())
  {
  for (int row=0; row<rows; row++)
  {
    if (row%2==0)
    {
    cols=20;
    }
    else
   {
     cols=15;
   }
    for (int col=0; col <cols; col++)
    {
      file >> a[row][col];
    }
  }
  file.close();
}
}

void print (int**a, int rows, int cols)
{
  for (int row=0; row<rows; row++)
  {
    for (int col =0; col <cols;col++)
    {
      cout <<setw(4) << a[row][col];
    }
    cout<<endl;
  }
}

int main()
{
 int numRows=20;
  int numCols = 15;

  int *a[numRows];
  for (int row=0;row<numRows; row++)
    a[row]=new int [numCols];

  fillarray(a,numRows, numCols);
  print (a,numRows, numCols);

  cout<<endl;
  
  return 0;
}

.txt file:

15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 
15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 
15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 
15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 
10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 
10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 
10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 
10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 
10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 
8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 
8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 
8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 
8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 
8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00 8.00
Ryan
  • 21
  • 1
  • 1
    You use an *array-of-pointers* (e.g. `int *a[numRows];` instead of a *pointer-to-pointer* (e.g. `int **a;`). So when you say you need to use "double-pointers", you haven't started off that way. Are you supposed to use an array of pointers, or are you supposed to allocate pointers and then a block of integers for each pointer? Also your array of pointers is a VLA which isn't part of C++ and only provided by non-standard extensions. – David C. Rankin Jun 19 '21 at 23:27
  • 1
    Many of your lines of input have more than `numCols = 15` columns... You want to read each line and parse each numeric value from the line, obtain the count, then allocate for that many and copy the values to storage. (you can use a normal temporary array for that purpose (e.g. `double tmp[100];` would do) Why are you using `int` at all when the values are *floating-point* values? – David C. Rankin Jun 19 '21 at 23:28
  • 1
    Also, I guess the purpose of your exercise is to dynamically allocate, otherwise you would simply use a `std::vector>` and let the container take care of the memory for you. I guess you haven't read [Why !.eof() inside a loop condition is always wrong.](https://stackoverflow.com/q/5605125/9254539) – David C. Rankin Jun 19 '21 at 23:32
  • Thank you so much, it's starting to work now! – Ryan Jun 19 '21 at 23:40
  • Glad to help. Don't forget to check on the rows exceeding `numCols` problem as well to make sure you get all the seat values. – David C. Rankin Jun 19 '21 at 23:47
  • About [`using namespace std`](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice)... – Aconcagua Jun 19 '21 at 23:48

1 Answers1

0

Continuing from the comments, as you work toward finishing your code, let me leave you with a few thoughts. Understand, that dynamically allocating storage for plain-old arrays has been replaced by the containers provided by STL. For example in this case, you would now simply declare a vector of vectors double and let the container handle all memory management for you (e.g. see std::vector)

That said, there is a lot of what is called "legacy code" that still exists that make it important for you to know how to properly use dynamic allocation. You can read the use of the new and delete expression -- there are important caveats related to when delete for delete[] must be used.

In short summary of the issue with your original code, the condition on your read loop is one of the primary traps new programmers fall into. Why !.eof() inside a loop condition is always wrong..

int *a[numRows]; is an array-of-pointers of fixed size that cannot be resized. Further you declare a as a VLA (Variable Length Array) which are not part of the C++ language. (and only provided by some compilers as a non-standard extension)

When you declare numCols = 15 and then size a with it, when you attempt to read lines with more than 15 columns into a you invoke Undefined Behavior attempting to write beyond the column bounds. (very bad things can happen, see: Undefined, unspecified and implementation-defined behavior and What is indeterminate behavior in C++ ? How is it different from undefined behavior? and Undefined behavior)

Now returning to how to read you file into a. The reason double pointers (e.g. a pointer-to-pointer-to double in this case) are used are so you can reallocate more pointers when needed. That is the whole point of dynamic allocation, you can resize the allocation and handle however many lines or columns of data you have without knowing how many you have to begin with.

Understand that a "double pointer", e.g. a pointer-to-pointer is a Single pointer. It holds as its value a memory address to where a block of one or more pointers are stored. (e.g. it points to where a block of pointers is located in memory). When using a pointer-to-pointer to store values simulating a 2D array, it is a two-step allocation process.

First you allocate some number of pointers assigning the starting memory address to the pointer (a in your case). Then you will allocate a block of memory holding some number of double values and assigning the starting address for that block to the next available pointer. So you will have one allocated pointer for each row of data and you will have one allocated block of double for each pointer holding the values from that line.

(it works the same for whatever type your pointer-to-pointer is, allocate pointers, then a block of memory of type for each pointer)

You can now index your collection of rows and values as you would a 2D array, (e.g. a[i][j]), but understand there is no array involved and there is no guarantee that the blocks of double allocated and assigned to a[i] and a[i+1] will be contiguous in memory. (with an actual 2D array they would be)

Other issues in your code, do NOT use MagicNumbers or hardcode filenames. You should not have to re-compile your code simply to read from another file. That is what argc and argv are for in int (int argc, char **argv). 15, 20 and "prices.txt" are all examples of what not to do. If you need a constant value, either #define one, or in C++, declare a constexpr, e.g.

constexpr int maxseats = 100;   /* constant, no. elements in temporary array */

Pass the filename to read from as the first argument to your program, or prompt the user for the name and read it as input. (it's much more convenient to simply provide the filename as an argument), e.g.

int main (int argc, char **argv)
{
  int numRows = 0;
  double **a = nullptr;
  
  if (argc < 2) { /* validate 1 argumemnt provided for filename */
    std::cerr << "error: insufficient arguments, filename required.\n";
    return 1;
  }
  
  std::ifstream f (argv[1]);
  if (!f.is_open()) {
    std::cerr << std::string{"error: file open faied '"} + argv[1] + "'.\n";
    return 1;
  }
  ...

If you note above, the file stream is opened and validated to be open in main() (the "caller" or "calling function") before the call to fillarray() is made. If you cannot open the file in the caller, then there is no reason to make the function call to begin with.

When your functions handle input, or do any operation that is critical to the continued operation of your program, you must choose a meaningful return type that can indicate whether the function succeeded or failed. If fillarray() doesn't fill a, you don't want to blindly try and print from a afterwards.

Instead, have fillarray() return something useful, like the number of rows of data actually filled in reading from the file. That way, the only parameters you need to provide are the open file stream and a. It will return the number of rows filled (and since you are allocating -- there is a subtle approach you can take to capture the number of columns in each row), e.g.

/* pass open stream, reference to a as arguments.
 * columns per-row will be stored as 1st element in row.
 * return number of rows.
 */
int fillarray (std::ifstream &f, double**& a)
{
  std::string line;
  int rows_allocated = 2,               /* no. of pointers allocated */
      rows_used = 0;                    /* no. of pointers filled / used */
  
  a = new double*[rows_allocated];      /* alloc initial 2 pointers */
  
  while (getline (f, line)) {           /* read line */
  ...
    rows_used += 1;                     /* increment rows_used counter */
  }
  return rows_used;                     /* return no. of rows filled */
}

Now in main() you can use the return from fillarray() to set numRows and validate the operation of fillarray(),

  ...
  numRows = fillarray (f, a);           /* fill a, save numRows */
  
  if (!numRows) { /* validate rows in a filled */
    std::cerr << "error: fillarray() returned zero.\n";
    return 1;
  }
  print (a, numRows);                   /* print stored values */
  ...

As mentioned above, using a pointer-to-pointer, you do not need to know how many lines you will read or rows you will fill (e.g. the number of pointers you will need) before you start. Simply allocate for some number of pointers, and then keep track of the number allocated and number used. The when used == allocated reallocate more pointers, copy the original pointers to the new block, delete[] the old, and assign the new to your original pointer. (basically a manual realloc()).

For the number of columns needed, you don't need an even/odd scheme (though you can use that if required). A very good approach where you know that the number of columns will be less than X is simply to use a temporary array to hold all values until you reach the end of the line. Then knowing how many elements you have allocate for that number and copy from the temporary array to the block you allocate. That allows for a very efficient 1-pass through the file and single allocation for each row.

Employing that approach in fillarray(), you could do:

/* pass open stream, reference to a as arguments.
 * columns per-row will be stored as 1st element in row.
 * return number of rows.
 */
int fillarray (std::ifstream &f, double**& a)
{
  std::string line;
  int rows_allocated = 2,               /* no. of pointers allocated */
      rows_used = 0;                    /* no. of pointers filled / used */
  
  a = new double*[rows_allocated];      /* alloc initial 2 pointers */
  
  while (getline (f, line)) {           /* read line */
    int n = 0;                          /* col count */
    double tmp[maxseats] = {0};         /* temp array of doubles */
    std::stringstream ss (line);        /* create stringstream from line */
    
    while (ss >> tmp[n])                /* read from stringstream into tmp */
      n++;                              /* increment column count */
    
    if (rows_used == rows_allocated) {  /* reallocate more pointers if req'd */
      /* allocate new block of pointers with twice the number of pointers */
      double **newptrblk = new double*[rows_allocated * 2];
      /* copy pointers from a to newptrblk */
      std::memcpy (newptrblk, a, rows_used * sizeof *a);
      delete[] a;                       /* delete memory used by a */
      a = newptrblk;                    /* assign new block of ptrs to a */
      rows_allocated *= 2;              /* update no. of pointers allocated */
    }
    
    a[rows_used] = new double[n + 1];   /* allocate for n + 1 double */
    a[rows_used][0] = n;                /* store no. of cols as 1st element */
    /* copy all values in tmp to a[rows_used] starting at 2nd element */
    std::memcpy (&a[rows_used][1], tmp, n * sizeof a[rows_used][1]);
    
    rows_used += 1;                     /* increment rows_used counter */
  }
  
  return rows_used;                     /* return no. of rows filled */
}

Now recall, to know how many columns we had per row, we used a subtle approach of allocating 1-more column for each row than needed. This allows us to store the number of columns as the first element in each row. This way without knowing anything about the number of columns you will have except that it will be less than some maximum number, you can handle all number of elements from 1 to max number. This requires a slight adjustment to your print() function to use the first element as the number of elements, e.g.

oid print (double **a, int rows)
{
  std::cout << std::setprecision(2) << std::fixed;  /* set desired format */
  
  for (int row = 0; row < rows; row++)  /* loop over each row */
  { /* output values beginning with element 1 */
    for (int col = 1; col < static_cast<int>(a[row][0]); col++)
    {
      std::cout << std::setw(6) << a[row][col];
    }
    std::cout.put('\n');
  }
}

Putting it altogether, you could write you code similar to the following:

#include <iostream>
#include <iomanip>
#include <fstream>
#include <sstream>
#include <string>
#include <cstring>

constexpr int maxseats = 100;   /* constant, no. elements in temporary array */

/* pass open stream, reference to a as arguments.
 * columns per-row will be stored as 1st element in row.
 * return number of rows.
 */
int fillarray (std::ifstream &f, double**& a)
{
  std::string line;
  int rows_allocated = 2,               /* no. of pointers allocated */
      rows_used = 0;                    /* no. of pointers filled / used */
  
  a = new double*[rows_allocated];      /* alloc initial 2 pointers */
  
  while (getline (f, line)) {           /* read line */
    int n = 0;                          /* col count */
    double tmp[maxseats] = {0};         /* temp array of doubles */
    std::stringstream ss (line);        /* create stringstream from line */
    
    while (ss >> tmp[n])                /* read from stringstream into tmp */
      n++;                              /* increment column count */
    
    if (rows_used == rows_allocated) {  /* reallocate more pointers if req'd */
      /* allocate new block of pointers with twice the number of pointers */
      double **newptrblk = new double*[rows_allocated * 2];
      /* copy pointers from a to newptrblk */
      std::memcpy (newptrblk, a, rows_used * sizeof *a);
      delete[] a;                       /* delete memory used by a */
      a = newptrblk;                    /* assign new block of ptrs to a */
      rows_allocated *= 2;              /* update no. of pointers allocated */
    }
    
    a[rows_used] = new double[n + 1];   /* allocate for n + 1 double */
    a[rows_used][0] = n;                /* store no. of cols as 1st element */
    /* copy all values in tmp to a[rows_used] starting at 2nd element */
    std::memcpy (&a[rows_used][1], tmp, n * sizeof a[rows_used][1]);
    
    rows_used += 1;                     /* increment rows_used counter */
  }
  
  return rows_used;                     /* return no. of rows filled */
}

void print (double **a, int rows)
{
  std::cout << std::setprecision(2) << std::fixed;  /* set desired format */
  
  for (int row = 0; row < rows; row++)  /* loop over each row */
  { /* output values beginning with element 1 */
    for (int col = 1; col < static_cast<int>(a[row][0]); col++)
    {
      std::cout << std::setw(6) << a[row][col];
    }
    std::cout.put('\n');
  }
}

int main (int argc, char **argv)
{
  int numRows = 0;
  double **a = nullptr;
  
  if (argc < 2) { /* validate 1 argumemnt provided for filename */
    std::cerr << "error: insufficient arguments, filename required.\n";
    return 1;
  }
  
  std::ifstream f (argv[1]);
  if (!f.is_open()) {
    std::cerr << std::string{"error: file open faied '"} + argv[1] + "'.\n";
    return 1;
  }
  
  numRows = fillarray (f, a);           /* fill a, save numRows */
  
  if (!numRows) { /* validate rows in a filled */
    std::cerr << "error: fillarray() returned zero.\n";
    return 1;
  }
  print (a, numRows);                   /* print stored values */
  
  for (int i = 0; i < numRows; i++)     /* loop freeing each block of doubles */
    delete[] a[i];
  delete[] a;                           /* free pointers */
}

Example Use/Output

With your data in the file dat/rowprices.txt, you would run the program and receive the following output:

$ ./bin/rowprices dat/rowprice.txt
 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00
 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00
 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00
 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00 15.00
 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00
 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00
 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00
 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00
 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00 10.00
  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00
  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00
  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00
  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00
  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00  8.00

Memory Use/Error Check

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to ensure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind ./bin/rowprices dat/rowprice.txt
==3927== Memcheck, a memory error detector
==3927== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==3927== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==3927== Command: ./bin/rowprices dat/rowprice.txt
==3927==
 <snipped - output>
==3927==
==3927== HEAP SUMMARY:
==3927==     in use at exit: 0 bytes in 0 blocks
==3927==   total heap usage: 283 allocs, 283 frees, 100,414 bytes allocated
==3927==
==3927== All heap blocks were freed -- no leaks are possible
==3927==
==3927== For counts of detected and suppressed errors, rerun with: -v
==3927== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

As often happens, this post ended up much longer than anticipated, but with the confusion shown in your use of an array-of-pointers instead of a pointer-to-pointer, there is really no short explanation that does the difference justice. There is no requirement you do things just this way or that you handle the number of elements per-row as I've done. That is the beauty of code, you are free to put the pieces together in whatever way you like -- so long as it works correctly.

Look things over and let me know if you have further questions.

David C. Rankin
  • 81,885
  • 6
  • 58
  • 85