current community

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

I'm quite new with C++ and trying to write a program at which I should:

  • take 20 numbers from the user,
  • putting them in an array,
  • writing a function to find the most repeated element,
  • returning the most repeated element,
  • printing it in the main function.

For that purpose I wrote the below code, but the output is the last number which is entered regardless to whether it's the most repeated one or not. 

#include<iostream.h>
#include<conio.h>

using namespace std ; 

int maxi ( long a[] , int size )
{
    int i , j , k=1 , max=0  , m ; 
    for (i=1 ; i<=size ; i++)
    {
        for (j=1 ; j<20 ; j++)
            {
             if (a[i] == a[j+1])k=k+1 ;
             } 
        if (max<k){ max = k ;  m=a[i] ;} 
    }
  return m ; 
}

int main ()
{
    const int size=20 ;
    long a[size] ; 
    cout << " Please Enter the elements of your aray " << endl ; 
    for (int i=1 ; i<=size ; i++ )
    {
        cout << "a["<<i<<"] : " ; 
        cin >> a[i] ; 
    }
    cout << "The number which does have the most repitition is : "<<maxi (a,size)  ; 
    getch() ; 
 return 0 ; 
}

I don't really understand what's wrong here. I would really appreciate it if anyone can help me to figure out what should change here.

share|improve this question
1  
Debugging questions should be on StackOverflow (according to the FAQ). I've flagged it, so moderators probably will migrate it to there soon. –  palacsint Jan 27 '12 at 21:37
 
+1, what @Loki Astari said. a similiar problem which might help you can be found here codereview.stackexchange.com/questions/7552/… –  bamboon Jan 27 '12 at 22:35
add comment

closed as off topic by seand, sepp2k♦ Jan 28 '12 at 2:38

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

2 Answers

up vote 2 down vote accepted

You should be creating and initializing k inside the outer loop; because you never reinitialize it, it will always be greater than the current max after the inner loop executes.

share|improve this answer
 
Thank you very much –  Negin Jan 27 '12 at 21:54
add comment
up vote 3 down vote

Your loop has a complexity of O(n^2) when it should be O(n).

Basically you run two loops inside each other. This kills your applications complexity. You just need to run over the array once then for each element increments its count (using a map to store the count). Then you just need to find the largest count and print the value.

share|improve this answer
 
storing the value of every element and later check which is the greatest doesn't seem like good idea. Isn't it much better to just set a max var, then update it when it finds a value that is greater than it? –  ajax333221 Jan 27 '12 at 22:52
 
@ajax333221: You can't set a max for any element without counting them all first. So you need to count each element (ie count all the 1's then count all the 2's). This give you a complexity of O(n^2) (which is the original algorithm above). The standard way of doing this is to keep a track of the count of each element as you go. Thus you only need to run across the container once to count them (then you run across the counts to find a max (which is less than or equal to the number of elements). Thus a complexity of O(n). –  Loki Astari Jan 27 '12 at 23:14
 
@ajax333221: What you are doing is swapping space for time. You need another container to store the counts which will take up a max of n locations in memory (if you have one of each element) but the usual is to expect more than one so you usually expect less than n. So if you're memory size is less than 2n you may want to re-think the algorithm. –  Loki Astari Jan 27 '12 at 23:15
 
oh, I thought the thing was about the greatest value, But now I see it is about the most repeated. –  ajax333221 Jan 27 '12 at 23:15
add comment

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