Better way to use a pointer?

Question

I'm trying to create a program that will display bar graphs with * the maximum number of * can be 40. I have everything working but had a question with the code. Is there a better way as you can see I have to go back to the original address twice using:

    p_bar_length = p_bar_length - size;

Is there a better way to do this?

#include <iostream>

using namespace std;

const int MAX_SPLATS = 40;

void bar_chart(double values[], int size)
{
    double largest = values[0]; //assign first element to be the largest

    //Find the largest value
    for (int i = 1; i < size; i++)
    {
        if (largest < values[i]) // check to see if there is something larger
        {
            largest = values[i];
        }
    }

    // Find the number of spalts to use
    // with the precent based on the largest value
    int* p_bar_length = new (nothrow) int[size];
    for (int i = 0; i < size; i++)
    {
        *p_bar_length = (values[i] / largest) * MAX_SPLATS;
        p_bar_length++; // Go to next memory address
    }

    // Go back to the orignal memory address
    p_bar_length = p_bar_length - size;

    // Pritnt the correct number of splats
    for (int i = 0; i < size; i++)
    {
        for (int j = 0; j < *p_bar_length; j++)
        {
            cout << "*";
        }
        p_bar_length++;
        cout << endl;
    }

    // Go back to the orignal memory address
    p_bar_length = p_bar_length - size;

    delete[] p_bar_length;
}

int main()
{
    double values[6] = { 22, 40, 28, 26, 14, 46};
    int val_size = 6;

    bar_chart(values, val_size);

    system("pause");

    return 0;
}

Answer 1

This is rather similar to the post from @mkaes, but goes one step further. Instead of using std::transform to create a vector of the proper lengths, then std::for_each to create a string the proper length from each of those, this creates a string directly from the input, and writes the strings directly from std::transform:

#include <iostream>
#include <array>
#include <algorithm>
#include <string>
#include <iterator>

const int MAX_SPLATS = 40;

template <typename C>
void bar_chart(const C& values)
{
    if (std::distance(values.begin(), values.end())<1)
        return; // do some error handling

    auto largest = *std::max_element(values.begin(), values.end());

    std::transform(values.begin(), values.end(), 
        std::ostream_iterator<std::string>(std::cout, "\n"), 
        [=](double d) { return std::string((d/largest)*MAX_SPLATS, '*');} );
}

int main() {
    std::array<double, 6> values = {22, 40, 28, 26, 14, 46};

    bar_chart(values);

    return 0;
}

Since it's using C++11 anyway, I decided to also use an std::array since it seems to fit nicely for the job at hand.

Answer 2

Since you tagged this as C++ I would recommend using the standard library.

Your program is more C than C++, but if c is ok for you there is not much to improve.
On the other hand, using vector and algorithm you don't need to mess around with pointers. And using C++11 it removes the rough edges previously associated with templates and iterators.

A quick shot:

#include <iostream>
#include <vector>
#include <algorithm>

const int MAX_SPLATS = 40;

template <typename C>
    void bar_chart(const C& values)
{
    if (std::distance(values.begin(), values.end())<1)
        return; // do some error handling

    auto largest = *std::max_element(values.begin(), values.end());

    // Find the number of splats to use with the percent based on
    // the largest value
    std::vector<int> bars(values.size());
    std::transform(values.begin(), values.end(), bars.begin(),
        [=] (double d) { return (d/largest)*MAX_SPLATS; });

    // Print the correct number of splats
    std::for_each(bars.begin(), bars.end(), 
        [](int val){ std::cout << std::string(val, '*') << std::endl; });
}

int main()
{
    std::vector<double> values = { 22, 40, 28, 26, 14, 46 };

    bar_chart(values);

    std::cin.get();
    return 0;
}

Answer 3

You do not need the first for loop if you use std::max_element from <algorithm>.

You do not need the second for loop if you calculate the bar length in the third for loop.

Something like this:

void bar_chart(double values[], int size)
{
    //Find the largest value
    double largest = *std::max_element(values, values + size);

    // Print the correct number of splats
    for (int i = 0; i < size; i++)
    {
        int p_bar_length = (values[i] / largest) * MAX_SPLATS;
        cout << string(p_bar_length, '*') << endl;
    }
}

That way you don't need the p_bar_length array at all. It is only a simple int.

Edit: And you could even replace the inner for loop (example modified)

Answer 4

How about two in one?

int* p_bar_length = new (nothrow) int[size];
for (int i = 0; i < size; i++)
{
    *p_bar_length = (values[i] / largest) * MAX_SPLATS;
     for (int j = 0; j < *p_bar_length; j++) cout << "*";
     cout << endl;
     p_bar_length++; // Go to next memory address
}

Answer 5

Rather than incrementing the pointer, use the array index:

p_bar_length[i] = (values[i] / largest) * MAX_SPLATS;

or use pointer arithmetic:

*(p_bar_length + i) = (values[i] / largest) * MAX_SPLATS;

Answer 6

You could treat p_bar_length as an array and just use consistent notation

int* p_bar_length = new (nothrow) int[size];
for (int i = 0; i < size; i++)
{
    p_bar_length[i] = (values[i] / largest) * MAX_SPLATS;
}

Answer 7

Since this is C++, the best way is not to use pointers; instead, use a std::vector.

That said, you can also always treat a pointer as an array and just access p_bar_length[i] for a given position 0 <= i < length instead of incrementing the pointer.

Answer 8

Make another copy of the pointer to use within your loop. Much less error prone.

int* p_bar_length = new (nothrow) int[size];
int* p = p_bar_length;
for (int i = 0; i < size; i++)
{
    *p = (values[i] / largest) * MAX_SPLATS;
    p++; // Go to next memory address
}

P.S. Why are you using nothrow? Since you're not checking the value you get back from new, an exception will be much nicer than the mess you'll have when you get back a NULL pointer.