6
\$\begingroup\$

I wrote a program that places values in one array to the end of another array. It keeps the order of the values. If the second array is bigger, then there will be leading zeroes.

Is there anything I can improve? How do I simplify this program?

#include <iostream>

    int main()
    {
        int fromArray[10] = {11,22,33,44,55,66,77,88,99,100};
        int toArray[15] = {};

        int size = sizeof(fromArray)/sizeof(fromArray[0]);

        int * first = fromArray;
        int * last = first + size; //One past end of array
        int * toLast = toArray + sizeof(toArray)/sizeof(toArray[0]); //One past end of array


        for(first; first != last;)
            *(--toLast) = *(--last);


        for(int i=0; i<sizeof(toArray)/sizeof(toArray[0]); i++)
            std::cout << toArray[i] << " ";



    }
\$\endgroup\$
5
\$\begingroup\$
  1. The first for loop is unnecessary. You can use a while loop instead.

    while(first != last)
        *(--toLast) = *(--last);
    
  2. Consider using a range-based for loop (C++11 only) for outputting values.

    for(auto i : toArray)
    std::cout << i << " ";
    
  3. Your program can be simplified using std::vectors, iterators and the function std::copy_backward(). In fact, what you are doing with the pointers pretty much is what iterators are meant to do.

    #include <iostream>
    #include <algorithm>
    #include <vector>
    #include <iterator>
    
    int main()
    {
        std::vector<int> fromVector {11,22,33,44,55,66,77,88,99,100};    
        std::vector<int> toVector(15); 
    
        std::copy_backward(fromVector.begin(), fromVector.end(), toVector.end());
    
        std::copy(toVector.begin(),toVector.end(),std::ostream_iterator<int>(std::cout, " "));
    }
    
\$\endgroup\$
6
\$\begingroup\$

Be aware that sizeof is actually an operator, not a function. It also returns std::size_t, which is an unsigned integer type. You currently have size as an int, which is a signed type. If you had your compiler warnings up high (and they should be), then you probably would've been warned about this.

std::size_t size = sizeof(fromArray)/sizeof(fromArray[0]);
\$\endgroup\$
5
\$\begingroup\$
for(int i=0; i<sizeof(toArray)/sizeof(toArray[0]); i++)

You should avoid doing the same math operation for every iteration of the loop unless the result will change based on something that happens in the loop. In this case, you are only reading from the array. So instead you can say

for ( std::size_t i=0, n = sizeof(toArray) / sizeof(toArray[0]); i < n; i++ )

This idiom is quite common in C. Note that I'm not arguing against the range-based for loop proposed by EngieOP. That obviously will avoid this specific problem in this case. However, there may be other circumstances where that won't help you. Another solution for this specific problem would be to use the size() method of the vector object:

for ( std::size_t i=0, n = toArray.size(); i < n; i++ )

You don't need to calculate the number of elements manually.

\$\endgroup\$

Your Answer

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