Why my sort function not working?

Question

2.00/5 (2 votes)

See more:

Everything is going right in program but the arrays are not getting sorted

What I have tried:

C++

#include <iostream>

using namespace std;
int price;
int n;
int* p_array=NULL;
int* p_matrix=NULL;

void swap(int *i,int *k)
{
    int j;// js any temperory element
    *i=*k;
    *k=j;
}
void sort(int _array[],int n )
{
    int hole;
    int i;
    hole = i;
    for(int hole=1;hole<n;hole++)>
    {
        if( hole>0 && _array[hole-1]>_array[hole])
        {
            swap(_array[hole],_array[hole-1]);
            hole=hole-1;
        }
    }
}


int main()
{
    p_array=new int[n];
    p_matrix=new int[n];
    cin >> n;
    for(int i=0;i<n;i++)>
    {
        cin >> *(p_array+i) ;
    }
    sort (p_array,n);
    for(int i=0;i<n;i++)>
    {
        *(p_matrix+i)=(*(p_array+i))*(n-i);
    }
    sort(p_matrix,n);
    for(int i=0;i<n;i++)>
    {
         cout << *(p_matrix+i)<< " " ;
         cout << endl;
         cout << *(p_array+i)<< " " ;
    }
}

Posted 25-Jun-16 0:44am

Member 12566145

Updated 25-Jun-16 4:40am

Andreas Gieriet

v4

Add a Solution

Comments

Andreas Gieriet 25-Jun-16 7:05am

Compile it with all warnings on!
E.g. n is not set but you create an array with n elements...
Why do you use pointer arithmetic when you could do it with far less pain with array indices? E.g. cin >> p_array[i];
Cheers
Andi

Philippe Mori 25-Jun-16 9:08am

- Don't use global variables.
- Initialize your variables when you declare them if initial value is known. Otherwise, declare the variable just before it initialization.
- Use appropriate spacing between operators.
- Do not start a name with an underscore (reserved for system use).
- Delete memory you allocate. Better yet, use std::vector instead of raw array.
- Prefer array notation to pointer notation.
- Use consistent spacing between functions (always 1 line or always 2 lines).

By the way, if you would have written a unit test for your swap function, you would have seen that it does not works as expected. The by using a debugger, you would have seen the problem. But for learning purpose, it is probably best to use pencil and paper.

3 solutions

Solution 2

C++

p_array=new int[n];
p_matrix=new int[n];
cin >> n;

That will never work how you expect. You allocate two arrays based on the value of variable n before you have assigned a value to it. You need to go through your code carefully to check the logic.

Posted 25-Jun-16 2:13am

Richard MacCutchan

Solution 3

You should learn to use the debugger as soon as possible. Rather than guessing what your code is doing, It is time to see your code executing and ensuring that it does what you expect.

The debugger allow you to follow the execution line by line, inspect variables and you will see that there is a point where it stop doing what you expect.
Debugger - Wikipedia, the free encyclopedia[^]
Mastering Debugging in Visual Studio 2010 - A Beginner's Guide[^]

The debugger allow you to see which part of your program conforms to your expectations, and which part don't. This allow you to narrow the research of bugs.

Posted 25-Jun-16 4:40am

Patrice T

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

OriginalGriff · Accepted Answer · 2016-06-25T01:03:00

Solution 1

Indentation helps a lot when you look at code: it means you can see what is goign on a lot more clearly...
But a quick glance says that the swap function:

C#

void swap(int *i,int *k)
{int j;// js any temperory element
*i=*k;
*k=j;
}

Is not going to work!
What value do you put in k each time?
Try changing it to:

C#

void swap(int *i,int *k)
    {
    int j;
    j = *i;
    *i = *k;
    *k = j;
    }

And that at least will work better!

In future, use the debugger: it would have shown you that problem in seconds, instead of the minutes it took you to ask here, then the time you had to wait for us to see the question, then the time for you to get an email that there is an answer...

Posted 25-Jun-16 1:03am

OriginalGriff

Comments

Member 12566145 25-Jun-16 7:15am

still NOT working :(

Michael_Davies 25-Jun-16 7:26am

Please show the output that would help to see what is wrong, the swap function Griff shows you should work fine so there is something else in your code, why do you hole - 1 if you swap?

OriginalGriff 25-Jun-16 7:58am

So use the debugger, and follow your code through.
It should be fairly obvious if you work out what you expect to happen, then compare that to what does happen when you single step each line.

Member 12566145 25-Jun-16 8:12am

THANK YOU there were 2 mistakes
1 of swap function
2 of hole=hole-1 (instead hole=0 must be there)

OriginalGriff 25-Jun-16 8:25am

You're welcome!
It's well worth playing with the debugger on little projects like this - it's a tool which will save you significant amounts of time and hair pulling when you move to more complex projects.