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] << " ";
 }
asked Oct 13, 2014 at 0:09
\$\endgroup\$

3 Answers 3

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, " "));
    }
    
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
answered Oct 13, 2014 at 0:13
\$\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]);
answered Oct 13, 2014 at 0:56
\$\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.

answered Oct 13, 2014 at 3:31
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.