-2
\$\begingroup\$

I have a C++ program where the input are 2 arrays, each containing a and z number of elements. The program then creates all possible combinations using all elements in both arrays. For example, if one array had 2^4 elements and the other had 2^7 elements, the total possible combinations would be (16 - 1) * (128 - 1) = 1,905.

Finding all combinations of the arrays can be done in a relatively quick time either by printing to console or writing the combinations to a file. The problem seems to be when writing the array to vector at the end main.cpp as this significantly slows the program after each iteration.

main.cpp

double ZProfile1Array[MPROFILEVAL];
double AProfile1Array[MPROFILEVAL];

int solver(char *filename){
  char zLine[MCOMBIN][300];
  FILE *plist;
  int i = 0; int stotal = 0;

  // Get array of possible combinations into zLine array
  char *sdfilename = getDSFileName(1);
  plist = fopen(sdfilename,"r");
  while(fgets(zLine[i],sizeof(zLine[i]),plist)){
    zLine[i][strlen(zLine[i]) - 1] = '\0'; // gets rid of \n that fgets gets
    i++;
  }
  fclose(plist);
  stotal = i;

  // Get array of possible  combinations into aLine array
  sdfilename  = getDSFileName(2);
  char aLine[MCOMBIN][300];
  i = 0; int dtotal = 0;
  plist = fopen(sdfilename,"r");
  while(fgets(aLine[i],sizeof(aLine[i]),plist)){
    aLine[i][strlen(aLine[i]) - 1] = '\0'; // gets rid of \n that fgets gets
    i++;
  }
  fclose(plist);
  dtotal = i;

  std::vector<double> itemA; 
  std::vector<double> itemZ; 
  std::string aCombo;
  std::string zCombo;

  char *token;char *tokend;char tstr[300];
  int index = 0;
  int j = 0;int id = 0; int jd = 0;int indexd = 0;

  // for each value of Sigma nA
  for(i = 1; i < zTotal; i++){ // i = 1 because first line is always empty
    zCombo = "";
    for(j = 0; j < rows; j++)ZProfile1Array[j] = 0;
    token = strtok(zLine[i]," ");
    index = 0;
    while (token){
      index = atoi(token) - 1; // because array indicies start from 0
      token = strtok(NULL," ");  
      for(j = 0; j < rows; j++){
        ZProfile1Array[j] += zprofile[index][j];
      }
      zCombo.append(sname[index]);
      zCombo.append(" + ");
    }
    zCombo.resize(zCombo.size() - 3); // remove trailing " + "

  // for each value of Sigma nZ
  for(id = 1; id < aTotal; id++){ // i = 1 because first line is always empty
    aCombo = "";
    for(jd = 0; jd < rows; jd++)AProfile1Array[jd] = 0;

    strcpy(tstr,aLine[id]); // use strtok on tstr because will be called a number of times and we don't want to modify aLine
    tokend = strtok(tstr," ");
    indexd = 0;

    while (tokend){
      indexd = atoi(tokend) - 1; // because array indicies start from 0 and nZ starts from 1
      tokend = strtok(NULL," "); 
      for(jd = 0; jd < rows; jd++){
        AProfile1Array[jd] += aprofile[indexd][jd];
      }
      aCombo.append(dname[indexd]);
      aCombo.append(" + ");
    }
    aCombo.resize(aCombo.size() - 3); // remove trailing " + "

  // write array to vector
  // *** next several lines slows down script ***
  std::fill(itemZ.begin(),itemZ.end(),0.0);
  std::fill(itemA.begin(),itemA.end(),0.0);
  for(int ii = 0; ii < rows; ii++){
    itemZ.push_back(ZProfile1Array[ii]);
    itemA.push_back(AProfile1Array[ii]);
  }
 printf("(%s,%s)\n",zCombo.c_str(),aCombo.c_str());
 }
}
}

Can the slowdown be avoided?

This is the core script of the program and calls on other functions from other files but I do not think they are relevant but can include them if necessary.

\$\endgroup\$
  • \$\begingroup\$ Are those std::fills correct? They zero out most of the data, except whatever was added on the last iteration \$\endgroup\$ – harold Dec 3 '19 at 15:35
  • 1
    \$\begingroup\$ To make your code ready for a humane code review, youshouldletyourIDEaddthemissingspacesaroundoperatorsandseveralotherplaces, because that makes the code more readable. \$\endgroup\$ – Roland Illig Dec 3 '19 at 16:24
  • 1
    \$\begingroup\$ The code doesn't compile for me since MPROFILEVAL is undefined. Please change your code so that it compiles, and provide some example data so that we can experiment and review the code under realistic conditions. \$\endgroup\$ – Roland Illig Dec 3 '19 at 16:26
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Dec 3 '19 at 16:51
  • 1
    \$\begingroup\$ You'll receive better reviews if you show a complete example. For example, I recommend that you edit to show the necessary #include lines, and a main() that shows how to call your function. It can really help reviewers if they are able to compile and run your program. \$\endgroup\$ – Toby Speight Dec 3 '19 at 16:52
2
\$\begingroup\$

That code is so convoluted (Looks like C).

To read a file into a vector of double.

#include <vector>
#include <iterator>
#include <fstream>

...

std::ifstream         file("filename");
std::vector<double>   data(std::istream_iterator<double>{file},
                           std::istream_iterator<double>{});

Done.


Vectors resize and copy there content from the old to new data space if they run out of room. To prevent this you can reserve enought space for all elements so that no reallocation happens.

std::vector<double> itemA;
std::vector<double> itemZ;

// You don't specify where rows is defined or set.
// But we know that these arrays will eventually reach this size.

itemA.reserve(rows);
itemZ.reserve(rows);

These lines are doing nothing useful:

std::fill(itemZ.begin(),itemZ.end(),0.0);
std::fill(itemA.begin(),itemA.end(),0.0);

It this point both vectors have zero elements and thus it does nothing.


This is not C++

printf("(%s,%s)\n",zCombo.c_str(),aCombo.c_str());

Stop using features from other languages. C++ has much better console output operations.

std::cout << "(" << zCombo << "," << aCombo << ")\n";
| improve this answer | |
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.