Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The purpose of the following code is to get a random number of 100 nodes and to distribute these nodes randomly in range 500*500...(X,Y). This was the first step:

#include<iostream>  
#include <fstream>  
#include<cmath>  

using namespace std;  

int  main()
{
 const int x = 0, y = 1;
 int nodes[100][2];
 ofstream myfile;
 myfile.open ("example.txt");
 myfile << "Writing this to a file.\n";

 for (int i=0; i<100 ;i++)  
 {     
      nodes[i][x] = rand() % 501;    
      nodes[i][y] = rand() % 501;  
      myfile <<nodes[i][x]<<" "<<nodes[i][y];  
 }

 myfile.close();
}

Now the next step is to improve this code to distribute these nodes in order (I must divide both xy_coordinates as : x = 0-100-200-300-400-500 & y=0-100-200-300-400-500) next is to distribute the nodes (regardless number of nodes) in order range starting from (0,100 )...(100,100)...(100,200)... until I reach the last point (500,500)).

I'm really confused of how to do it correctly. I start to think to define a 2 dimensional array and then define two for loops:

Int no_nodes=100;  
    Int  XY_coordinate [500][500];  
     For (int i=0;i<no_nodes; i++)  
  {  
 For (int j=0;j<no_nodes; j++)

As shown in the image, the x and y axis will be divided in to 5*5, so I will have 25 grids Starting the axis from 0-99-199-299 until 499. What I'm trying to do is to distribute 4 nodes in each "100*100" grid.

I've tried to do it by rewriting the code, but I get an error and I could not understand it:

1 IntelliSense: expression preceding parentheses of apparent call must have (pointer-to-) function type

How can I fix it?

enter image description here


   #include <iostream>  
   #include <fstream>  
   #include <cmath>  

    using namespace std;  



     int  main()   

     {  
 ofstream myfile;  
       myfile.open ("example.txt");  
       myfile << "Writing this to a file.\n"; 


       int miny=0,max_y=99;
       for(int i=0;i<5;i++)
       {
int minx=0,max_x=99;
       for(int j=0;j<5;j++)
      {
for(int k=0;k<=4;k++)
{
int     x=(minx+rand()*10(max_x +1)%(max_x - minx+1));
int     y=(miny+rand()*10(max_y +1)%(max_y - miny+1));
}
minx=max_x+1;
max_x=max_x+100;
     }
     miny=max_y+1;
     max_y=max_y+100;
     }
      }
share

migration rejected from programmers.stackexchange.com Dec 26 '13 at 18:25

This question came from our site for professional programmers interested in conceptual questions about software development. Votes, comments, and answers are locked due to the question being closed here, but it may be eligible for editing and reopening on the site where it originated.

closed as off-topic by Jamal Dec 26 '13 at 18:25

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Your question must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After getting your code to work, you may edit this question seeking a review of your working code." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

    
What are you trying to do here? Are you having trouble implementing your functionality or do you have working code that you want to be critiques? Questions about implementations are off-topic here, but if you make it more clear what your problem is, we can migrate your question to a better place. –  Thomas Owens Nov 3 '12 at 17:43
    
I need any suggestion to improve the code , from distributing the nodes randomly in (500,500) range to distribute them in order in the x-y axis . –  Laian Alsabbagh Nov 3 '12 at 18:03
1  
Very first comment: Please do try to be more consistent in your indentation. In parts you indent with 1 space, in other parts 5 spaces, in parts 1 space less than the line preceding. The most common styles I've seen use 4 spaces. 3 is also common. It's all a matter of taste, but 1 is definitely too small, and inconsistent spacing makes code hard to read. –  asveikau Nov 4 '12 at 21:25
comments disabled on deleted / locked posts

1 Answer 1

At least for now, I'm just going to look at coding style.

#include<iostream>  
#include <fstream>  
#include<cmath>  

I'd always put a space before the <. Never putting a space there is somewhat acceptable too. Changing from one to the other is not so good.

using namespace std;  

This should almost always be avoided. It's generally better to include the namespace qualifiers as needed, or just use using declarations to bring in the names you really need.

int  main()   
{  
 const int x = 0, y = 1;   
 int nodes[100][2];

Here you're basically using a 2D array to simulate an array of 2D points. I'd rather see 2D points. Since you're going to create points with values, let's include a constructor to make that a little simpler:

struct point { 
    int x, y;

    point(int x, int y) : x(x), y(y) {}
};

Since you're also writing these values out to a stream, let's overload operator<< for the type:

std::ostream &operator<<(std::ostream &os, point const &p) { 
    return os << p.x << " " << p.y;
}

I'd also rather see a vector than an array:

std::vector<point> nodes;

When it comes to creating and initializing objects:

 ofstream myfile;  
 myfile.open ("example.txt");

...prefer to create an object initialized over creating it uninitialized, and initializing it to a useful state later:

std::ofstream myfile("example.txt");

This doesn't make a huge difference in this case, but I'd try to cultivate the habit -- in other cases it makes a much bigger difference (and even in this case, it's at least slightly helpful).

 for (int i=0; i<100 ;i++)  
 {     
     nodes[i][x] = rand() % 501;    
     nodes[i][y] = rand() % 501;  
     myfile <<nodes[i][x]<<" "<<nodes[i][y];  

With the definitions we have for point, nodes and operator<< we have above, this loop becomes something like:

for (inti=0; i<100; i++) {
    nodes.push_back(point(rand() % 501, rand() % 501));
    myfile << nodes[i];
}

You might also want to consider using algorithms for the job instead:

std::generate_n(std::back_inserter(nodes), 100, []{return node(rand()%501, rand()%501); });
std::copy(nodes.begin(), nodes.end(), std::ostream_iterator<node>(myfile, "\n"));

Finally (well, final comment on this point, anyway) % doesn't produce a particularly good distribution of numbers either. You might want to consider another method.

myfile.close();

I generally prefer to let RAII do its job, so manually closing a file is unnecessary. Better to let it close automatically as the stream object goes out of scope.

share
    
I don't understand what you do in your review with std::generate and std::copy ?! –  Stephane Rolland Nov 4 '12 at 11:41
    
+1 for mentioning that the obvious approach of "% 501" actually doesn't provide a good distribution of values. I went for years without considering this... –  Dan Nov 4 '12 at 14:38
    
@StephaneRolland: Part of what I did was leave out one of the parameters to generate_n. Maybe it's now more understandable (and thanks for drawing my attention to it). –  Jerry Coffin Nov 4 '12 at 15:28
    
oki, better now corrected for the generate_n(). Thanx for the std::copy with stream iterator, I had never used that this way. Great to learn that :-) –  Stephane Rolland Nov 4 '12 at 17:26
    
Rather than recommending bizzare-looking loops, and since you're using C++11 lambdas already, wouldn't it be better to use <random>? –  conio Nov 29 '12 at 1:36
show 2 more comments

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