Good or bad code?

[mlt]

I am writing a function that takes a reference P to a
std::vector<std::vector<int> >. Before calling this function I don't know
the dimensions of P. I need to define those in the function and I need to
read form all elements in P. Here is what I do:

void testVector(std::vector<std::vector<int> > & P)
{

// Currently hardcoded
int rows = 3;
int columns = 3;

// Initializing vectors of vectors
P.resize(rows);
for (int i=0; i<rows; i++)
{
P.resize(columns);
for (int j=0; j<columns; j++)
{
P[j] = 33;
}
}

// Reading from vector of vectors.
int a = 0;
for (int i=0; i<rows; i++)
{
for (int j=0; j<columns; j++)
{
P[j] = a;
a++;
}
}


}

int main() {

std::vector<std::vector<int> > P;
testVector(P);

int rows = P.size();
int columns = P[0].size();
// Printing
for (int i=0; i<rows; i++)
{
for (int j=0; j<columns; j++)
{
std::cout << P[j] << std::endl;
// P[j] = 33;
}
}

return 0;

}


But is this how it should be done? Seems a bit clumsy to me.

 

[Martin Eisenberg]

I am writing a function that takes a reference P to a
std::vector<std::vector<int> >. Before calling this function I
don't know the dimensions of P. I need to define those in the
function and I need to read form all elements in P.

Must the composite object always be rectangular? If so, you shouldn't
be handling the nested vector explicitly. Rather, encapsulate it in a
class that ensures all components are the same length, and lets you
query the bounds.

void testVector(std::vector<std::vector<int> > & P)

Side remark: That function doesn't test anything, so better call it
makeTestVector().


Martin

 

[Noah Roberts]

I am writing a function that takes a reference P to a
std::vector<std::vector<int> >. Before calling this function I don't
know the dimensions of P. I need to define those in the function and I
need to read form all elements in P. Here is what I do:

void testVector(std::vector<std::vector<int> > & P)
{

// Currently hardcoded
int rows = 3;
int columns = 3;

// Initializing vectors of vectors
P.resize(rows);
for (int i=0; i<rows; i++)
{
P.resize(columns);
for (int j=0; j<columns; j++)
{
P[j] = 33;
}
}

// Reading from vector of vectors.
int a = 0;
for (int i=0; i<rows; i++)
{
for (int j=0; j<columns; j++)
{
P[j] = a;
a++;
}
}


}

The above function has two separate and distinct responsibilities as is
even pointed out by your comments. This is bad. Split this function
into two separate functions.

 

[Martin Eisenberg]

mlt wrote:

void testVector(std::vector<std::vector<int> > & P)
{

// Currently hardcoded
int rows = 3;
int columns = 3;

// Initializing vectors of vectors
P.resize(rows);
for (int i=0; i<rows; i++)
{
P.resize(columns);
for (int j=0; j<columns; j++)
{
P[j] = 33;
}
}

// Reading from vector of vectors.
int a = 0;
for (int i=0; i<rows; i++)
{
for (int j=0; j<columns; j++)
{
P[j] = a;
a++;
}
}


}

The above function has two separate and distinct
responsibilities as is even pointed out by your comments.

Actually the second comment is just wrong as it's (somewhat
pointlessly) filling twice.


Martin