0
\$\begingroup\$

This prints out a triple layer circle with 12 points per circle and finds the points across a grid.

I had a really hard time with this and I'm planning to implement a lot more code but I don't want to spoil my end result. I'm new so take it easy on me but feel free to criticize as I am looking to improve my coding and syntax.

#include <iostream>
#include <algorithm>
#include <cmath>

struct vec2{
double circ;
double x;
double y;
} v[36];

void circle(){
double pi = 3.141;
double r = 6.75;
for (int i = 0; i < 3; i++) {
    v[i].circ = (2*pi)*r;
    std::cout << "circ # " << i+1 << ": " << v[i].circ << std::endl;

    for (int j = 0; j < 12; j++)  {
         int w = 19;
         int h = 12;
         int comp = j*30;
        double rad = comp * (pi / 180);
        double x = w + (r * cos(rad)) * 5/3;
        double y = h + (r * sin(rad));
        int fact = i*12+j;
        v[fact].x=x;
        v[fact].y=y; 
        }    r += 2; 
    std::cout << std::endl;
    }}

bool vecfind(int row, int col){
for(int i = 0; i<36; i++){
    if(v[i].x >= col && v[i].x  < col+1 && 
                v[i].y >= row && v[i].y < row+1)
    { return true;}
    }
    return false;
    }

void square(){
for(int row = 0; row < 23; row++){  
    for(int col = 0; col < 39;col++){
        if (vecfind(row, col)){std::cout << "x";}
        else{std::cout << " ";}}
std::cout << std::endl;}
return;}

int main() {
circle();
square();

return 0;} 

output:

circ # 1: 42.4035

circ # 2: 54.9675

circ # 3: 67.5315


                  x                    
          x                x           
                  x                    
           x              x            
                  x                    
   x         x          x         x    
      x                        x       
         x                  x          



 x  x  x                      x  x  x  


         x                  x          
      x                        x       
   x         x          x         x    
                   x                   
           x              x            
                   x                   
          x                x           
                   x                   
\$\endgroup\$
  • \$\begingroup\$ I edited my post. Can this post be unheld? \$\endgroup\$ – Richard Christopher Oct 4 '18 at 1:28
  • 1
    \$\begingroup\$ Does this indentation reflect how you wrote the code? If now, the easiest way to post code is to paste it into the question editor, highlight it, and press Ctrl-K to mark it as a code block. \$\endgroup\$ – 200_success Oct 4 '18 at 3:58
  • \$\begingroup\$ I see that you haven't kicked your habit of using global variables. \$\endgroup\$ – 200_success Oct 4 '18 at 4:05
  • \$\begingroup\$ I'm doing all of this on mobile.. and I am still learning. For the sake of this small expirimentation - global access of v was needed to make designing and testing (and learning) simple enough. Achieving the results was hard enough. \$\endgroup\$ – Richard Christopher Oct 4 '18 at 12:13
3
\$\begingroup\$

There's a lot that can be improved in your code, especially if you are using C++. The more advanced features of C++ actually help you organize your code better, help you write less code to do what you want, and remove a lot of tedium that you find in C-style code.

Use proper names for variables and functions

Names should accurately reflect what the purpose is of a variable or a function. Of course, names should be concise, but apart from some well established conventions such as using i for a loop index, or x and y for coordinates, don't abbreviate names. Here are some suggestions for improvements in your code:

  • Instead of circle(), name the function draw_circles().
  • Instead of square(), name the function print_result().
  • Instead of comp, name the variable angle, or perhaps phi.

Use '\n' instead of std::endl

std::endl is in fact equivalent to '\n', but in addition it also forces a flush of the output stream, which will slow down the output if it is used a lot.

Don't store unnecessary data

You have added a member variable circ to struct vec2. First of all, it is rather strange to name something vec2 when it contains 3 elements. But it is also unnecessary to store the radius of the circle in that struct at all.

Avoid using global variables

As already mentioned in the comments, don't use global variables if you can easily avoid them. Keep the array v inside main(), and pass it as a parameter to the other functions. It does not seem like a big deal now, but once your code grows you will appreciate keeping the global namespace tidy, and by passing references/pointers to your data to functions, you make those functions more reusable.

Stick to radians when possible

All trigonometric functions in <cmath> use radians, and any calculations done involving angles are usually simpler when done in radians than in degrees. Instead of:

int comp = j*30;
double rad = comp * (pi / 180);

Write this:

double angle = j * (2 * pi / 12);

Use a proper value for π

If your <cmath> library provides it, use M_PI for the value of π, or if not, write double pi = std::acos(-1) (see this question). Just don't make unnecessary approximations; it costs you nothing to do it properly, and you never know when an approximation might actually cause you problems.

Store the image, not the coordinates of the X'es.

Building a list of coordinates of where the X'es are, and then checking for each position in the output whether it matches the coordinates of one of the X'es is not efficient. This algorithm scales with O(width * height * number_of_Xes). You either want to write the X'es directly to the screen at the right position, or keep an array of width * height characters in memory, write the X'es to that array, and then at the end print the contents of that character array. Let's use the latter method, which brings us to:

Use a class to represent the output image

You are using C++, so make use of its features! Make a class that manages an ASCII-art image for you, like so:

#include <cassert>
#include <iostream>
#include <vector>

class Image {
    std::vector<char> pixels;

    public:
    const unsigned int width;
    const unsigned int height;

    // Create a w * h image, fill it with spaces
    Image(unsigned int w, unsigned int h):
            width(w), height(h), pixels(w * h, ' ') {}

    // Return a reference to the character at coordinates x, y
    char &at(unsigned int x, unsigned int y) {
        assert(x < width && y < height);
        return pixels[y * width + x];
    }

    // Print the image
    void print(std::ostream &out) {
        for (unsigned int y = 0; y < height; y++) {
            for (unsigned int x = 0; x < width; x++)
                out << at(x, y);
            out << '\n';
        }
    }
};

This class will store the characters that make up the image in a std::vector, which acts mostly like an array, but it has the nice property that it also allocates and cleans up memory for you behind the scenes.

The function at() will return a reference to the character in the vector at the given coordinates. You can use this to both read and write to that character.

The function print() takes a std::ostream reference as an argument, so you can have it print to any stream you want.

Using this class you can create an ASCII-art image, draw to it, and then print it like so:

int main() {
    // Create a 10x10 character image
    Image image(10, 10);

    // Draw a diagonal line
    for (int i = 0; i < 10; ++i)
        image.at(i, i) = '\\';

    // Print the results to the standard output
    image.print(std::cout);
}

Now you can rewrite your circle() function to write to a class Image, like so:

void draw_circle(Image &image, double radius, int steps) {
    static const double pi = std::acos(-1);

    for (int i = 0; i < steps; ++i) {
        double phi = i * (2 * pi / steps);
        double x = image.width / 2 + radius * cos(phi);
        double y = image.height / 2 + radius * sin(phi);
        image.at(x, y) = 'X';
    }
}

And then you can draw 3 circles with increasing radii like so:

int main() {
    Image image(40, 40);

    for (auto radius: {8, 12, 16})
        draw_circle(image, radius, 12);

    image.print(std::cout);
}

Of course, this will look like an ellipse on the screen, so you will have to find some way to correct the aspect ratio. You could create a draw_ellipse() function which takes two parameters for the radius, one for the horizontal direction and another for the vertical.

\$\endgroup\$
  • \$\begingroup\$ G. Sliepen comes in with the Hail Mary! Big ups and thank you for taking your time!! \$\endgroup\$ – Richard Christopher Oct 4 '18 at 12:28
  • \$\begingroup\$ @RichardChristopher: I'm glad you like the answer. If it was helpful, consider marking it as the accepted answer (see stackoverflow.com/help/someone-answers). \$\endgroup\$ – G. Sliepen Oct 4 '18 at 17:32

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.