5
\$\begingroup\$

Any way to make this more concise/efficient?

// Write a loop that reverses the elements of an array.
#include <iostream>
#include <iomanip>

const int SIZE = 10;

void reverse(int iArrayRef1 []);
void display(int iArrayRef2 []);

int main()
{
    int iArray[SIZE] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

    // Display in the original order.
    std::cout << "Forwards:\n";
    display(iArray);

    // Reverse the array.
    reverse(iArray);

    std::cout << std::endl;

    // Display in reversed order.
    std::cout << "Backwards:\n";
    display(iArray);
    return 0;
}

void reverse(int iArrayRef1 [])
{
    int temp = 0;
    int* indexPtrBeg = iArrayRef1;
    int* indexPtrEnd = iArrayRef1 += (SIZE-1);

    for (int index = 0; index < (SIZE/2); index++)
    {
        temp = *indexPtrEnd;
        *indexPtrEnd = *indexPtrBeg;
        *indexPtrBeg = temp;
        indexPtrEnd--;
        indexPtrBeg++;
    }
}

void display(int iArrayRef2 [])
{
    for (int index = 0; index < SIZE; index++)
    {
        std::cout << std::setw(2) << iArrayRef2[index] << " ";
    }
    std::cout << std::endl;
}
\$\endgroup\$
9
  • \$\begingroup\$ I don't see much wrong. I only wonder why you call the argument iArrayRef1 in reverse and iArrayRef2 in display. One other minor thing: ++index is slightly faster than index++ since the post-increment operator has to return the pre-incremented value (it first makes a copy, then increments and returns the copy). This also goes for indexPtrEnd-- of course. Modern compilers might optimize the copy away if you don't use it, but it won't hurt to use pre-increment when possible. \$\endgroup\$
    – thelamb
    Oct 19, 2013 at 19:38
  • \$\begingroup\$ Thank you! In regards to the parameter list, I am still unsure about how to name similar parameters, is there a convention that people generally stick to? \$\endgroup\$ Oct 19, 2013 at 19:50
  • \$\begingroup\$ There's nothing against using the same name in both functions. Personally, all of my function arguments start with 'a' (void display( int aArray[] )) to distinguish them from local variables. Also I don't believe hungarian notation (prepending type information to variable names, in your case iArray, where i stands for int I suppose) is very common in C++. In the end though, naming style is personal and you should pick your own and stick to it. Inconsistency is the biggest mistake you can make :). \$\endgroup\$
    – thelamb
    Oct 19, 2013 at 20:00
  • \$\begingroup\$ @thelamb: Yes, normally Hungarian is discouraged. :-) Starting them with a isn't best either, but I don't know if it's discouraged as well. You also won't really need to worry about "local" naming unless you're dealing with classes (you'll have local and member). \$\endgroup\$
    – Jamal
    Oct 19, 2013 at 20:03
  • \$\begingroup\$ @thelamb: I doubt there is any difference in speed between ++index and index++. With built-in types there will be no difference. And this is also a bad argument to use. The argument you should be using is that the speed is not the same for all types and for user defined types in general it is slower (because of the reason you stated). So for maintenance purposes prefer the prefix version so that if the types change your code is always using the optimum version. \$\endgroup\$ Oct 20, 2013 at 22:35

4 Answers 4

12
\$\begingroup\$

For an implementation without the STL, this is pretty good. There's no risky memory-management going on with the raw pointers, and your sorting isn't needlessly slow (compared to bubble sort). I'll base my review on the STL anyway (for future reference), along with some other general tips.


  • SIZE should be of type std::size_t. It's preferred to use an unsigned integer type for sizes, plus you'll have a larger range (no negative values are part of it).

  • If you have C++11, prefer std::array (or any other STL container) over C-style arrays. This also makes it needless to pass the array size since STL containers already know their own.

    Your array initialization, for instance, would look like this:

    std::array<int, SIZE> iArray = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
    

    If you use the above array or another STL container, the loop counters in reverse() and display() should be of type std::size_type. This will ensure:

    1. the supported size type for this container is used
    2. the loop will be able to access any number of elements

    Also, as @thelamb mentioned, pre-increment (++index) is preferred because it will avoid extra copies. This is especially important for user-defined types (such as std::array), while post-increment is still okay with native types (such as int).

  • Prefer iterators to raw pointers in C++:

    // with C++11
    
    auto begin = std::begin(container);
    auto end   = std::end(container);
    

    // without C++11
    
    std::ContainerType::iterator begin = std::begin(container);
    std::ContainerType::iterator end   = std::end(container);
    
  • You could compare your reverse algorithm to the idiomatic STL method with regards to efficiency (if you were to perform a speed test on them):

    std::reverse(container.begin(), container.end());
    
  • I'd avoid the extra newlines in display(). The function's purpose is solely to display the array, plus you'll be forced to have the newline with each call. Two things for this:

    1. add the newlines before and/or after the function call instead
    2. use \n instead of std::endl (the latter also flushes the buffer, which takes longer)

    So, instead of this:

    std::cout << std::endl;
    
    std::cout << "Backwards:\n";
    display(iArray);
    

    you could have this:

    std::cout << "\nBackwards:\n"; // another '\n' at the front
    display(iArray);
    std::cout << "\n";             // newline here instead of in display()
    

For comparison, here's an idiomatic alternative using templates (any element type is supported). It uses an std::vector instead of an array, saving you size constraints. It also displays the vector in forward and in reverse using an iterator, instead of mutating it for the reverse.

#include <algorithm> // std::copy, std::reverse_copy
#include <iostream>
#include <iterator>  // std::ostream_iterator
#include <vector>

template <typename T>
void displayForward(std::vector<T> const& vec)
{
    std::copy(vec.cbegin(), vec.cend(),
        std::ostream_iterator<T>(std::cout, " "));
}

template <typename T>
void displayReverse(std::vector<T> const& vec)
{
    std::reverse_copy(vec.cbegin(), vec.cend(),
        std::ostream_iterator<T>(std::cout, " "));
}

int main()
{
    std::vector<int> vec = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

    std::cout << "Forward:\n";
    displayForward(vec);

    std::cout << "\nBackwards:\n";
    displayReverse(vec);
}
\$\endgroup\$
7
  • \$\begingroup\$ Thanks Jamal! I am familiar with the STL std::array, in my class however my teacher requested we use the c-style arrays. If given the choice though, would it be better to use a std::array or a std::vector/std::list? \$\endgroup\$ Oct 19, 2013 at 19:57
  • \$\begingroup\$ Oh, okay. I didn't know that. Anyway, that depends on how you will use your data. If you just need something like a C-style array (static, not dynamic), then choose std::array. If you need to use a dynamically-allocated array (allowing you to add/remove elements from the back), choose std::vector. If you're using a linked list (allowing you to add/remove elements anywhere, albeit possibly slowly), choose std::list. Also note that std::list specifically implements a doubly linked-list. \$\endgroup\$
    – Jamal
    Oct 19, 2013 at 20:01
  • \$\begingroup\$ As for your implementation with disregard to the STL, I think it's okay. There's not much else you can do with C-style arrays and raw pointers. The code still compiles, displays the correct results, and looks clean. I'll add a few more things here. \$\endgroup\$
    – Jamal
    Oct 19, 2013 at 20:08
  • 1
    \$\begingroup\$ You can also pass an array by reference. void func(T (&param)[Size]). Useful when used in combination with templates. \$\endgroup\$ Oct 20, 2013 at 22:47
  • 1
    \$\begingroup\$ If you're stuck with C-style arrays but can still use the STL and have access to C++11 then you can use std::begin(array) and std::end(array) to use STL algorithms on C-style arrays. \$\endgroup\$
    – Jake Woods
    Oct 21, 2013 at 2:58
7
\$\begingroup\$

Don't declare an array with a specific size if you are also going to initialize it:

int iArray[SIZE] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

If SIZE is too small them you have an error (that is not reported). If SIZE is too large then the array is silently filled with 0. Both can be an issue. Prefer:

// Let the compiler work out the size from the data you provide.
int iArray[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

Though it looks like you are passing an array here:

void display(int iArrayRef2 [])

Its not. You are just passing a pointer (arrays decay to pointers). All array context is lost. This is dangerous as some people forget and try and use iArrayRef2 as if it was an array. So when you declare a function like this prefer to use pointer syntax. This will remind people that they are dealing with pointers.

void display(int* iArrayRef2)

Passing an array as a pointer without a size is a bad idea. You have no idea where it ends. Always pass size information. There are a couple of ways to do this:

void display(int* iArrayRef2, int size)     // Standard C way
void display(int* begin,      int* end)     // Standard C++ 03 way (iterator based)

// Or do it so you can allow real templates
template<typename I>
void display(I begin, I end)

// Or pass by reference
template<int size>
void display(int (&iArrayRef2)[size])

// Or pass any of the standard containers.

Note if you use the template version above:

display(&iArray[0],  &iArray[SIZE]);   // prints forward
display(std::reverse_iterator<int*>(&iArray[SIZE]), std::reverse_iterator<int*>(&iArray[0])); // prints backwards

You don't need to re-invent swap

    temp = *indexPtrEnd;
    *indexPtrEnd = *indexPtrBeg;
    *indexPtrBeg = temp;

    // or
    std::swap(*indexPtrBeg, *indexPtrEnd);

If you do encapsulate it in its own function. This will make reading the code easier.

Prefer to use std::reverse rather than your own version (its highly optimized).

There are some useful standard algorithms that can make the code shorter and more readable.

for (int index = 0; index < SIZE; index++)
{
    std::cout << std::setw(2) << iArrayRef2[index] << " ";
}

// or
std::cout << std::setw(2);
std::copy(&iArrayRef2[0], &iArrayRef2[SIZE], std::ostream_iterator<int>(std::cout, " "));

Example of passing an array by reference:

template<typename T, int size>
int size(T (&array)[size])
{
      return size;
}
int main()
{
      int   a[10];
      float b[14];
      std::cout << size(a) << " " << size(b) << "\n";
}
\$\endgroup\$
0
3
\$\begingroup\$

I'll just focus on your reverse() function, assuming that you want to keep the C-style interface rather than the C++-style std::array suggested by @Jamal. (It's an excellent suggestion, but I just want to focus on some basic ideas for this review.)

I would prefer to have the array size passed in explicitly, rather than relying on the SIZE constant. That way, you can pull the SIZE constant into main() rather than having it be global.

In int* indexPtrEnd = iArrayRef1 += (SIZE-1), the += is weird. Just + is sufficient; reassigning iArrayRef1 as a side-effect is confusing (but luckily harmless).

Since you already have two pointers crawling along the array in opposite directions, there is no need to introduce an index variable. You can just terminate the loop when indexPtrBeg and indexPtrEnd meet or cross each other.

I find your variable names cumbersome. How about array, begin, and end? (In indexPtrBeg, index... is just misleading, and ...Ptr... stinks of Hungarian notation.) The temp variable is only used within the loop, so you should declare it inside the loop.

#include <sys/types.h>   /* for size_t */

void reverse(int array[], size_t size) {
    for (int *begin = array, *end = array + size - 1; begin < end; ++begin, --end) {
        // You could use std::swap() from #include <algorithm>
        // std::swap(*begin, *end);
        int swap = *end;
        *end = *begin;
        *begin = swap;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ +1 good stuff. It's also nice having STL and non/STL reviews to consider. :-) I do have some notes. 1.) It seems that SIZE could still be global since it's used in more than one function. 2.) I would argue that the OP's incrementing and decrementing in reverse() is more readable. They do work in your example, but they seem to bloat that top line. That may just be me, though. \$\endgroup\$
    – Jamal
    Oct 20, 2013 at 1:45
1
\$\begingroup\$

since this is int reversing, you could avoid the pointers and temp values

(saves memory space, which in this case is not so important but this math trick might help you in the future)

and just use subtraction and adding

for example:

int a= 13;
int b= 25;
a=a+b;//38
b=a-b;//38-25 = 13
a=a-b;//38-13 = 25

all you have to do is take an array of size x

and you reverse the values of the First(0 index) and last(x-1 index)

then the second (1 index) and one before the last(x-2 index)

and so on

EDIT:

here is my solution using this math trick:

#include <iostream>
#include <iomanip>

const int SIZE = 10;

void reverse(int array[])
{
    for (int i = 0; i < SIZE/2; i++)
    {
        array[i]+=array[SIZE-(i+1)];
        array[SIZE-(i+1)]=array[i]-array[SIZE-(i+1)];
        array[i]-=array[SIZE-(i+1)];
    }
}

void display(int iArrayRef2 [])
{
    for (int index = 0; index < SIZE; index++)
    {
        std::cout << std::setw(2) << iArrayRef2[index] << " ";
    }
    std::cout << std::endl;
}

int _tmain(int argc, _TCHAR* argv[])
{
    int iArray[SIZE] = { 1, 2, 3, 4, 5, 6, 7, 8, 9,10 };

    // Display in the original order.
    std::cout << "Forwards:\n";
    display(iArray);

    // Reverse the array.
    reverse(iArray);

    std::cout << std::endl;

    // Display in reversed order.
    std::cout << "Backwards:\n";
    display(iArray);

    return 0;

}
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.