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] << " ";
}
3 Answers 3
The first
for
loop is unnecessary. You can use awhile
loop instead.while(first != last) *(--toLast) = *(--last);
Consider using a range-based
for
loop (C++11 only) for outputting values.for(auto i : toArray) std::cout << i << " ";
Your program can be simplified using
std::vector
s, iterators and the functionstd::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, " ")); }
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]);
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.