Logic Error? Swapping First And Last Elements and so on....

[akshatmahajan3112]

So It's 20 days in my C++ class and I have been getting along pretty fast so my teacher decided kick things up a notch for me and gave me 10 questions,5 for 2-D arrays and 5 for 1-D.There's this one Which I can't seem to solve. Here Goes The Question:

Swap The First And Last,Second And Second last elements of an array of numbers if either one (first or last Or second or second last or so on,one of them is odd)

Like This- Original Array : 5,16,4,7,19,8,2
Changed : 2,16,19,7,4,8,5

Here's My Code For It With Comments So As To Explain The Logic I Applied.

#include<iostream>
using namespace std;
int main()
{
    int arr[20],i,n,temp;
    cout<<"\nEnter The Number Of elements :";
    cin>>n;                      //Number Of Elements For The ARRAY,Suppose 7
    for (i = 0 ; i < n ; i++)
    {
        cin>>arr[i];             //Reading The ARRAY
    }
   n = n-1;
   for (i = 0 ; i < n ; i++,n--)
   {
      if (arr[i] % 2 != 0 || arr[n] % 2 !=0)
      {


       temp = arr[i];                //arr[0] goes into temp
       arr[i] = arr[n];            //arr[0] goes into arr[6]
       arr[n] = temp;              //arr[6] goes into temp or arr[0]
   }
   }
   for (i = 0 ; i < n ; i++)
   {
       cout<<"\n"<<arr[i];             //Output For The Changed Array
   }
   return 0;
}

I have ran out of ideas any help would be appreciated.

Thanks!

[icy1]

ok, a few tips

1. Avoid cin for now, especially while testing the logic; it'll just slow you down. Just hard code it.
2. Show the before and after states so it's clear.
3. Can also use std::swap , but the use of temporary values is a good learning exercise.

With the additions I mentioned:

#include <iostream>

using namespace std;

int main()
{
    int arr[] = {5, 16, 4, 7, 19, 8, 2};
    int arr_size = sizeof(arr)/sizeof(arr[0]);
    
    cout << "Original:" << endl;
    for (int i = 0 ; i < arr_size ; i++)
        cout << arr[i] << " ";
    cout << endl << endl;

    for (int i = 0, n = arr_size-1 ; i < n ; i++, n--)
    {
        if (arr[i] % 2 != 0 || arr[n] % 2 !=0)
        {
            swap(arr[i], arr[n]);
        }
    }
    
    cout << "Changed:" << endl;
    for (int i = 0 ; i < arr_size ; i++)
       cout << arr[i] << " ";             //Output For The Changed Array
    cout << endl;
    
    return 0;
}

Edit: I typo'd while refactoring, but I think you actually did solve it -- your original logic was sound. The problem you probably ran into was that n was decremented and so the second for loop did not output everything. Make a copy of the array size, as in the refactor above ^
Edit2: Added in swap.

Note: I'd like to point out if you eventually do try to generate dynamic arrays of integers with the help of user input, use vector<int>. You could run into issues in your original implementation if the user enters a number greater than 20.

[Handy Andy]

Hello akshatmahajan3112,

This is a good example of using a single letter for a variable. It works in a for loop, but not for what you are doing.

By the time you reach line 24 do you know what the value of "n" is?

This if over done, but does prove a point:

#include<iostream>
#include <limits>

//using namespace std;

constexpr int MAXSIZE{ 20 };

int main()
{
	int arr[MAXSIZE]{ 5, 16, 4, 7, 19, 8, 2 };
	int i{}, anountOfArrayUsed{ 7 }, temp{};  // <--- Always initialize your variables.

	//std::cout << "\nEnter The Number Of elements :";
	//std::cin >> anountOfArrayUsed;                      //Number Of Elements For The ARRAY,Suppose 7

	//for (i = 0; i < anountOfArrayUsed; i++)  // <--- Commented for testing.
	//{
	//	std::cin >> arr[i];             //Reading The ARRAY
	//}

	anountOfArrayUsed = anountOfArrayUsed - 1;
	
	for (i = 0; i < anountOfArrayUsed; i++, anountOfArrayUsed--)
	{
		if (arr[i] % 2 != 0 || arr[anountOfArrayUsed] % 2 != 0)
		{
			temp = arr[i];                //arr[0] goes into temp
			arr[i] = arr[anountOfArrayUsed];            //arr[0] goes into arr[6]
			arr[anountOfArrayUsed] = temp;              //arr[6] goes into temp or arr[0]
		}
	}

	for (i = 0; i < anountOfArrayUsed; i++)
	{
		std::cout << " " << arr[i] << ' ';             //Output For The Changed Array
	}

	// This next line may not be needed. If you have to press Enter to see the prompt then comment out the
	// next line. If not you will need this to clear the input buffer first.
	//std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');  // <--- Requires header file <limits>.
	std::cout << "\n\n Press Enter to continue";
	std::cin.get();

	return 0;
}

Now when you reach line 33 you can see the "anountOfArrayUsed" has a smaller value than when it started.

The logic for the swap is sound it is the for loops that are off.

In the first for loop you are using "i" to check the first number or element 0 and "anountOfArrayUsed" for the last element of the array, but it is not the last element because have subtracted 1 before you even use it, so it is the next to last element that you are checking.

It would be better at line 21 to assign "anountOfArrayUsed" to a new variable to use in the for loop leaving "anountOfArrayUsed" unchanged.

At the beginning of the program when you prompt the user for a number of elements the prompt should include a range like 2 to 20 because you need at least 2 for the program to work and you can not go over 20. This should be followed with a check to make sure the input is with in range.

You do have a good program it just needs a little fixing.

Hope that helps,

Andy

[keskiverto]

The Standard Library does have a function for reversal. It is described in: http://www.cplusplus.com/reference/algorithm/reverse/
Alas, it has "templates" and "iterators", and lacks that conditional "swap if odd".

Nevertheless, it might become interesting when you feel like kicking the next notch.

[Handy Andy]

Hello akshatmahajan3112,

As I ran the program I found that the first for loop was swapping and then unswapping elements because the "i" iterator is going through the entire array when is should not.

This is what I ended up with:

lastElement = anountOfArrayUsed - 1;
	
for (i = 0; i < anountOfArrayUsed / 2; i++, lastElement--)
{
	if (arr[i] % 2 != 0 || arr[lastElement] % 2 != 0)
	{
		temp = arr[i];                        //arr[0] goes into temp
		arr[i] = arr[lastElement];            //arr[0] goes into arr[6]
		arr[lastElement] = temp;              //arr[6] goes into temp or arr[0]
	}

I added the variable "last Element" so that "anountOfArrayUsed" would not be changed. This works for both odd and even number arrays.

This should work better for what you want.

Hope that helps,

Andy

[keskiverto]

I feel playful:

template <class BidirectionalIterator>
  void reverse (BidirectionalIterator first, BidirectionalIterator last)
{
  while ((first!=last)&&(first!=--last)) {
    std::iter_swap (first,last);
    ++first;
  }
}

Only the odds

template <class BidirectionalIterator>
  void reverseodd (BidirectionalIterator first, BidirectionalIterator last)
{
  while ((first!=last)&&(first!=--last)) {
    if ( (*first % 2)||(*last % 2) ) {
      std::iter_swap (first,last);
    }
    ++first;
  }
}

From iterators to indices:

template <class ArrayType>
  void reverseodd ( ArrayType & array, size_t first, size_t last )
{
  while ( (first!=last)&&(first!=--last) ) {
    if ( (array[first] % 2)||(array[last] % 2) ) {
      swap( array[first], array[last] );
    }
    ++first;
  }
}

But indices have operator<

template <class ArrayType>
  void reverseodd ( ArrayType & array, size_t first, size_t last )
{
  while ( first < --last ) {
    if ( (array[first] % 2)||(array[last] % 2) ) {
      swap( array[first], array[last] );
    }
    ++first;
  }
}

No template, just array of int

void reverseodd ( int* array, size_t first, size_t last )
{
  while ( first < --last ) {
    if ( (array[first] % 2)||(array[last] % 2) ) {
      std::swap( array[first], array[last] );
    }
    ++first;
  }
}

[lastchance]

#include <iostream>
#include <algorithm>
using namespace std;

template <class RandomAccessIterator, class BinaryPredicate>
void reverse_if( RandomAccessIterator begin, RandomAccessIterator end, BinaryPredicate f )
{
   for ( --end ; begin < end; begin++, end-- ) if ( f( *begin, *end ) ) swap( *begin, *end );
}

int main()
{
   int arr[] = { 5, 16, 4, 7, 19, 8, 2 };
   cout << "\nOriginal: ";   for ( int i : arr ) cout << i << ' ';

   reverse_if( arr, arr + sizeof( arr ) / sizeof( arr[0] ), []( int a, int b ){ return a%2 || b%2; } );
   cout << "\nArranged: ";   for ( int i : arr ) cout << i << ' ';
}

Original: 5 16 4 7 19 8 2 
Arranged: 2 16 19 7 4 8 5 

[JLBorges]

> I feel playful

Ditto.

#include <iostream>
#include <iterator>
#include <algorithm>
#include <list>
#include <type_traits>

namespace utility
{
    namespace detail
    {
        template < typename BIDIRECTIONAL_ITERATOR >
        void reverse( BIDIRECTIONAL_ITERATOR begin, BIDIRECTIONAL_ITERATOR end, std::bidirectional_iterator_tag tag )
        {
            if( ( begin != end ) && ( begin != --end ) )
            {
                std::iter_swap( begin, end ) ;
                reverse( ++begin, end, tag ) ;
            }
        }

        template < typename RANDOM_ACCESS_ITERATOR >
        void reverse( RANDOM_ACCESS_ITERATOR begin, RANDOM_ACCESS_ITERATOR end, std::random_access_iterator_tag tag )
        {
            if( begin < --end )
            {
                std::iter_swap( begin, end ) ;
                reverse( ++begin, end, tag ) ;
            }
        }

        template < typename ITERATOR > using category = typename std::iterator_traits<ITERATOR>::iterator_category ;
        template < typename ITERATOR >
           constexpr auto swappable_values = std::is_swappable<typename std::iterator_traits<ITERATOR>::value_type>::value ;
        template < typename ITERATOR >
           constexpr auto nothrow_swappable_values = std::is_nothrow_swappable<typename std::iterator_traits<ITERATOR>::value_type>::value ;
    }

    template < typename ITERATOR >
    void reverse( ITERATOR begin, ITERATOR end ) noexcept( detail::nothrow_swappable_values<ITERATOR> )
    {
        static_assert( detail::swappable_values<ITERATOR> ) ;
        detail::reverse( begin, end, typename detail::category<ITERATOR>{} ) ;
    }

    template < typename SEQUENCE >
    void reverse( SEQUENCE& seq ) noexcept( detail::nothrow_swappable_values< decltype( std::begin(seq) ) > )
    { utility::reverse( std::begin(seq), std::end(seq) ) ; }
}

int main()
{
    int a[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 } ;
    utility::reverse(a) ;
    for( int v : a ) std::cout << v << ' ' ;
    std::cout << '\n' ;

    std::list b { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 } ;
    utility::reverse(b) ;
    for( int v : b ) std::cout << v << ' ' ;
    std::cout << '\n' ;
}

http://coliru.stacked-crooked.com/a/a4d6a76387734291

[akshatmahajan3112]

Oh God Guys! The Amount of knowledge or things I got cleared up with help of you all is unbelievable.Love This Forum.Thank You Very Much. Though Something went over my head hahahaha,Stil Much appreciated,My program Finally Worked!