-2

I am making a simple database system in C++. The table data is stored in a file, where each line represents a table row, where all data is separated by spaces. I want to read ncols elements in same line, where ncols is not always the same, and store each read value in data[x]. data variable declaration is char** data.

void Table::LoadTableRows(Table::TableStruct *table,char *dbname) {
    ifstream fp;
    Table::RowStruct *p = (Table::RowStruct*) malloc(sizeof(Table::RowStruct));
    char *filename;
    int x;
    filename = (char*) malloc((strlen(table->tablename)+strlen(dbname)+strlen("Data"))*sizeof(char));
    strcpy(filename,dbname);
    strcat(filename,table->tablename);
    strcat(filename,"Data");

    fp.open(filename);

    while(!fp.eof()) { //goes through all file lines
        Table::RowStruct *newrow = (Table::RowStruct*) malloc(sizeof(Table::RowStruct)); //allocates space for a new row
        //initializes element
        newrow->prev = NULL;
        newrow->next = NULL;
        newrow->data = (char**) malloc(table->ncols*30*sizeof(char)); //allocates space to store the row data
        for(x=0;x<table->ncols;x++) {
            newrow->data[x] = (char*) malloc(30*sizeof(char)); //allocates space for individual data element
            fp >> newrow->data[x];
        }
        for(p=table->rows;p->next!=NULL;p=p->next) {}
        newrow->prev = p;
        p->next = newrow;
    }

    fp.close();
}

I've tried this code, but it crashed as I expected.

  • 4
    See: [Why is iostream::eof inside a loop condition considered wrong?](https://stackoverflow.com/q/5605125/253056) – Paul R Jun 12 '19 at 21:36
  • 6
    Storing an unknown amount of values inside a container is what `std::vector` does best. – Some programmer dude Jun 12 '19 at 21:38
  • 4
    On that note, storing a string of `char`s of unknown size is what `std::string` does best. You generally shouldn't be using `malloc` in C++, or even raw pointers – alter_igel Jun 12 '19 at 21:44
  • 1
    In short, you are trying to do too many new things at once. I advise you to try them one at a time. – Beta Jun 12 '19 at 21:58
  • 1
    You may want to use `std::list` instead of developing and debugging your own linked list. – Thomas Matthews Jun 12 '19 at 22:06

2 Answers2

0

I do not fully understand what you want to do. There is missing information. Anyway. I will try to help.

I guess that you are new to C++. You are using a lot of C functions. And your program looks completely like C, with some additional C++ features. That you should not do. You are especially using malloc and raw pointers. This you must not do at all.

Try to learn C++ step by step.

Let me first show you what I mean with C-Style programming. I took your program and added comments with hints.


// Do not pass arguments by pointer, pass by reference
// For invariants, pass as const T&
// Do not use "char *". Should be at least const. But do not use at all
// Use std::string (so pass "const std::string& dbname") as argument
void Table::LoadTableRows(Table::TableStruct *table,char *dbname) {
    // Always initialize varibles. Use universal initialization, with {}
    ifstream fp;
    // Never use malloc. Use new.
    // Do not use raw ptr. use std::unique_ptr. Initialize with std::make_unique
    // Do not use C-Style cast. Use static_cast
    Table::RowStruct *p = (Table::RowStruct*) malloc(sizeof(Table::RowStruct));
    // Use std::string
    char *filename;
    int x;
    // Again. No malloc, no C-Style cast
    // Do not use C-Sytle string functions
    filename = (char*) malloc((strlen(table->tablename)+strlen(dbname)+strlen("Data"))*sizeof(char));
    // Do not use C-Sytle string functions
    strcpy(filename,dbname);
    // Do not use C-Sytle string functions
    strcat(filename,table->tablename);
    // Do not use C-Sytle string functions
    strcat(filename,"Data");
    // Check, if open works, Open file through constructor, then it will be closed by destructor
    fp.open(filename);

    while(!fp.eof()) { //goes through all file lines
        // Do not use malloc and C-Style cast
        Table::RowStruct *newrow = (Table::RowStruct*) malloc(sizeof(Table::RowStruct)); //allocates space for a new row
        //initializes element
        // Do not use NULL, but nullptr
        newrow->prev = NULL;
        newrow->next = NULL;
        // Do not use malloc and C-Style cast
        newrow->data = (char**) malloc(table->ncols*30*sizeof(char)); //allocates space to store the row data
        // Do not use x++ but ++x
        for(x=0;x<table->ncols;x++) {
        // Do not use malloc and C-Style cast
            newrow->data[x] = (char*) malloc(30*sizeof(char)); //allocates space for individual data element
            // Check for out of bounds
            fp >> newrow->data[x];
        }
        // Do not use selfmade linked list. Use STL container
        for(p=table->rows;p->next!=NULL;p=p->next) {}
        newrow->prev = p;
        p->next = newrow;
    }

    fp.close();
}

You see, there is a lot of C in it and not so much C++.

The modern C++ makes much use of containers and algorithms.

A full fledged example for C++ is below. It is hard to understand for beginners. But try to analyze and you will get a hang of it.

#include <vector>
#include <string>
#include <iostream>
#include <fstream>
#include <iterator>
#include <algorithm>
#include <sstream>


using AllWordsInOneLine = std::vector<std::string>;
using AllLines =std::vector<AllWordsInOneLine>;

struct Line      // ! This is a proxy for the input_iterator ! 
{   // Input function. Read on line of text file and split it in words
    friend std::istream& operator>>(std::istream& is, Line& line) {
        std::string wholeLine;  std::getline(is, wholeLine); std::istringstream iss{ wholeLine }; line.allWordsInOneLine.clear(); 
        std::copy(std::istream_iterator<std::string>(iss), std::istream_iterator<std::string>(), std::back_inserter(line.allWordsInOneLine));
        return is;
    }
    operator AllWordsInOneLine() const { return allWordsInOneLine; }  // cast to needed result
    AllWordsInOneLine allWordsInOneLine{};  // Local storage for all words in line
};


int main()
{
    std::ifstream inFileStream{ "r:\\input.txt" };      // Open input file. Will be closed by destructor
    if (!inFileStream) { // ! operator is overloaded
        std::cerr << "Could not open input file\n";
    }
    else {
        // Read complete input file into memory and organize it in words by lines
        AllLines allLines{ std::istream_iterator<Line>(inFileStream), std::istream_iterator<Line>() };

        // Make exact ncols entries. 
        const size_t ncols = 6; // whatever ncols may be. Empty cols will be filled with ___  (or whatever you like)
        std::for_each(allLines.begin(), allLines.end(), [ncols](AllWordsInOneLine& awil) {awil.resize(ncols, "___"); });

        // copy result to std::cout
        std::for_each(allLines.begin(), allLines.end(), [](AllWordsInOneLine & awil) {std::copy(awil.begin(), awil.end(), std::ostream_iterator<std::string>(std::cout, " ")); std::cout << '\n'; });
    }
    return 0;
}

Please see especially that the whole file, with all lines split into words, will be read in one line of code in function main.

An additional one-liner converts this into a vector with exactly ncols elements (words). This regardless if there were more or less than ncols words per line in the source file.

Hope I could help at least a little bit.

A M
  • 14,694
  • 5
  • 19
  • 44
0
char *filename;
filename = (char*) malloc((strlen(table->tablename)+strlen(dbname)+strlen("Data"))*sizeof(char));
strcpy(filename,dbname);
strcat(filename,table->tablename);
strcat(filename,"Data");

Here's your first problem. You haven't allocated space for the terminating nul byte at the end of the string. I'm not sure why you're using C-style strings instead of std::string, but C-style strings use a zero byte at the end to mark the end of the string.

fp.open(filename);

while(!fp.eof()) { //goes through all file lines

You are misusing eof. It can't predict that a future read will succeed, it's not a future-predicting function but a past reporting function.

    newrow->data = (char**) malloc(table->ncols*30*sizeof(char)); //allocates space to store the row data

This is puzzling. The type is char **, which means you're allocating a pointer to a pointer. Yet you allocate space for 30 characters. Why allocate 30 characters for a pointer?

        fp >> newrow->data[x];

You don't check if this read succeeds. That's never a good thing and makes your program impossible to debug.

These are the major issues that immediately stand out.

David Schwartz
  • 179,497
  • 17
  • 214
  • 278