6
\$\begingroup\$

I hope I've asked this in the right area. I'm writing a program that deals with enqueueing a class on STL queue in C++. Initially it queues initializations of the class. Then while the queue is not empty, it performs some operations on the class and then checking if an inequality works it either pops the queue or passes the class into function that creates more versions of the class and then queues the extra versions. It then pops the original initialization.

I'm trying to change this so it suits the format, I've also changed the title. At this stage I'm looking to minimize memory footprint mainly and maximize speed as a secondary. I'm basically looking for advice on lines I can remove or where it's inefficient. int main() would also include code to initially queue Subcubes. I hope this is more what you're looking for but if not just let me know.

Vec map(const Vec &g,const int &p);      \\I haven't added these as I dont think theyre massicaly important
double pnorm(const Vec &s,const int &p);
typedef vector<double> Vec;
typedef vector<Vec> Mat;
typedef queue< Subcube>  SUBQUEUE;

void subdivide(Subcube &nw, const Subcube &f ,const Vec &t)
{
 // cout << "running subdivide" << endl;

 Vec v = t;
 v.insert(v.begin() + f.k, 0);

int n = f.e.size();
nw.e.assign(n,0);  //just to avoid runtime error
nw.hside = (f.hside/2);
nw.k= f.k;


for(int j=0;j<n;j++)
{
    nw.e.at(j) = f.e.at(j) +(f.hside/2)*v.at(j);
    }
  }

int main(){
while(!q.empty())
{
    double upperb=0;
    upperb = 2*pow(double(n),0.25)*q.front().hside;
    Vec radmap = map(q.front().e,p);   // taking the vector from front subcube and maping it radially from the l-infy to the l4 unit ball
    double temp = pnorm(a*radmap,p);   // taking  a nomrm of matrix a multiplied by radmap
    if(temp>M)
        {M=temp;
        final = radmap;}               // to see what vector maximised it at the end
    if(temp>= M*(1+e)*(1-(((p-1)/2)*pow(upperb,2)))){
        int tr = pow(2,n-1);
        for(int j =0;j<tr;j++)
        {
            q.push(subdivide(q.front(),set.at(j))); // splits subcube into 2^n-1 subcubes
        }
        counter++;                  //counts related to number of subcubes created
       }
       q.pop();}
}

class Subcube
{
  public:
    Subcube();
    void printvector(const int& n);
    void Subcubeset(const int& i,const int& n);
    vector<double> e;
    double hside;
    char k;
    virtual ~Subcube();

protected:
private:
};  
\$\endgroup\$
6
  • 3
    \$\begingroup\$ This question might be better suited for StackOverflow. In either case, you should provide some code so others can see what the problem might be. \$\endgroup\$
    – tobias_k
    Jul 1, 2013 at 16:15
  • \$\begingroup\$ How much code should I put up? There's quite a lot of it :/ \$\endgroup\$
    – James
    Jul 1, 2013 at 18:02
  • \$\begingroup\$ How are you measuring memory usage? Are you expecting the memory to be returned to the system when you program frees it? That depends upon the memory allocator, but I would not be at all surprised if freeing memory had no effect on the memory usage of the process (from the OS perspective) - the allocator just keeps the memory for later use. \$\endgroup\$ Jul 1, 2013 at 19:41
  • \$\begingroup\$ I was using System Monitor. Okay that might make sense. Is there anyway to measure it within the program or return it to the system? There's millions initializations of the class and uses up all my ram before crashing if I try to solve something difficult \$\endgroup\$
    – James
    Jul 1, 2013 at 23:29
  • \$\begingroup\$ To answer "How much code should I put up?": It should be SSCCE. \$\endgroup\$
    – Lstor
    Jul 2, 2013 at 1:55

1 Answer 1

5
\$\begingroup\$

As @Lstor mentions in the comments, clean up your formatting and indentation. There should be plenty of resources online, and you can also look at other highly-developed code for ideas. Above all, keep it clear and consistent.

Naming

  • This isn't a very descriptive typedef:

    typedef vector<double> Vec;
    

    There's no point in using a typedef here if you're just going to name it what it already it. Name it based on how this type is used in your program, otherwise don't bother renaming it.

  • Why is this all uppercase?

    typedef queue< Subcube>  SUBQUEUE;
    

    It's a type, not a macro. Just like you have with Vec, have it start with an uppercase, but don't use all uppercase.

Functions

  • I'm not sure what this is:

    for(int j=0;j<n;j++)
    {
        nw.e.at(j) = f.e.at(j) +(f.hside/2)*v.at(j);
        }
      }
    

    For one thing, the second closing curly brace is for the function. But the poor indentation makes it appear to belong to the loop.

    Put some whitespace between the operators and operands within the loop statement. This should be done everywhere for clarity.

    Consider adding comments explaining what the loop is doing. I'm not even sure what is going on inside. Even if something seems obvious to you, it won't be obvious for everyone. Now, if something is too obvious, then it doesn't warrant commenting. But this does.

  • Many things from main() and below is very unclear. Not only should everything inside main() be indented, but it appears that you're missing a closing brace (the one above class looks like it belongs to the while loop).

    That's an odd place to declare the class. It should be put into a separate header file, with this file including the header. If you're going to have main() defined after everything else (which is good), make sure it is the last thing in the file.

Subcube

  • I'm not sure if you already have the implementation elsewhere and haven't included it here, but make sure you have something for it. It would be helpful to see how those data members are used and how those member functions perform.

  • Everything is crammed into public, and there are some things that shouldn't be there. The data members (k, hside, and e) should be under private. The functions and constructor should stay public, unless some shouldn't be used by outside code. If you're not using protected, just get rid of it. It doesn't need to be there for nothing.

  • In printvector() and Subcubeset(), you don't need to pass the ints by const&. Passing an int is relatively cheap, so just pass them by value.

  • printvector() should be const as it shouldn't be modifying any data members:

    void printvector(const int& n) const;
    
  • Subcubeset() should use camelCase naming as it's a function.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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