-1

I am trying to write a program to count each number the program has encountered. by putting M as an input for the number of the array elements and Max is for the maximum amount of number like you shouldn't exceed this number when writing an input in the M[i]. for some reason the program works just fine when I enter a small input like

Data input:

10 3
1 2 3 2 3 1 1 1 1 3

Answer:

5 2 3

But when I put a big input like 364 for array elements and 15 for example for max. the output doesn't work as expected and I can't find a reason for that!

#include "stdafx.h"
#include <iostream>
#include<fstream>
#include<string>
#include <stdio.h>
#include<conio.h>
using namespace std;

int  ArrayValue;
int Max;
int M[1000];
int checker[1000];
int element_cntr = 0;
int cntr = 0;
int n = 0;
void main()
{

    cout << "Enter the lenght of the Elements, followed by the maximum number: " << endl;
    cin >> ArrayValue>> Max;

    for (int i = 0; i < ArrayValue; i++)
    {
        cin >> M[i];
        checker[i]= M[i] ;
        element_cntr++;

        if (M[i] > Max)
        {
            cout << "the element number " << element_cntr << " is bigger than " << Max << endl;
        }


    }


    for (int i = 0; i < Max; i++)
    {
        cntr = 0;
        for (int j = 0; j < ArrayValue; j++)
        {

            if (M[n] == checker[j])
            {
                cntr+=1;

            }       

        }


        if (cntr != 0)
        {
            cout << cntr << " ";
        }
        n++;
    }





}
haitham hany
  • 33
  • 2
  • 11
  • 1
    1. [`void main()` is wrong](http://stackoverflow.com/q/204476/995714). 2. [“using namespace std;” considered bad practice?](http://stackoverflow.com/q/1452721/995714) – phuclv Aug 17 '15 at 04:23

1 Answers1

0

You have general algorithm problem and several code issues which make code hardly maintainable, non-readable and confusing. That's why you don't understand why it is not working.

Let's review it step by step.

The actual reason of incorrect output is that you only iterate through the first Max items of array when you need to iterate through the first Max integers. For example, let we have the input:

7 3
1 1 1 1 1 2 3

While the correct answer is: 5 1 1, your program will output 5 5 5, because in output loop it will iterate through the first three items and make output for them:

 for (int i = 0; i < Max; i++)
    for (int j = 0; j < ArrayValue; j++)
        if (M[n] == checker[j]) // M[0] is 1, M[1] is 1 and M[2] is 1

It will output answers for first three items of initial array. In your example, it worked fine because the first three items were 1 2 3.
In order to make it work, you need to change your condition to

if (n == checker[j]) // oh, why do you need variable "n"? you have an "i" loop!
{
    cntr += 1;
}    

It will work, but both your code and algorithm are absolutely incorrect...

Not that proper solution

You have an unnecessary variable element_cntr - loop variable i will provide the same values. You are duplicating it's value.

Also, in your output loop you create a variable n while you have a loop variable i which does the same. You can safely remove variable n and replace if (M[n] == checker[j]) to if (M[i] == checker[j]).

Moreover, your checker array is a full copy if variable M. Why do you like to duplicate all the values? :)

Your code should look, at least, like this:

using namespace std;

int ArrayValue;
int Max;
int M[1000];
int cntr = 0;

int main()
{

    cout << "Enter the lenght of the Elements, followed by the maximum number: " << endl;
    cin >> ArrayValue >> Max;

    for (int i = 0; i < ArrayValue; i++)
    {
        cin >> M[i];

        if (M[i] > Max)
        {
            cout << "the element number " << i << " is bigger than " << Max << endl;
        }
    }


    for (int i = 0; i < Max; i++)
    {
        cntr = 0;
        for (int j = 0; j < ArrayValue; j++)
        {
            if (i == M[j])
            {
                cntr ++;
            }      
        }

        if (cntr != 0)
        {
            cout << cntr << " ";
        }
    }

    return 0;
}

Proper solution

Why do you need a nested loop at all? You take O(n*m) operations to count the occurences of items. It can be easily counted with O(n) operations.

Just count them while reading:

using namespace std;

int arraySize;
int maxValue;
int counts[1000];

int main()
{

    cout << "Enter the lenght of the Elements, followed by the maximum number: " << endl;
    cin >> arraySize >> maxValue;

    int lastReadValue;

    for (int i = 0; i < arraySize; i++)
    {
        cin >> lastReadValue;

        if (lastReadValue > maxValue)
            cout << "Number " << i << " is bigger than maxValue! Skipping it..." << endl;
        else
            counts[lastReadValue]++; // read and increase the occurence count
    }


    for (int i = 0; i <= maxValue; i++)
    {
        if (counts[i] > 0)          
            cout << i << " occurences: " << counts[i] << endl; // output existent numbers
    }

    return 0;
}
Yeldar Kurmangaliyev
  • 33,467
  • 12
  • 59
  • 101
  • 1
    Oh. I will never understand StackOverflow. I have written a 110-lines answer to get a downvote and best answer mark. Any explanations, downvoter? – Yeldar Kurmangaliyev Aug 17 '15 at 04:16
  • Why the output is showing Maxvalue-1 it only prints the correct output but not the correct amount – haitham hany Aug 17 '15 at 04:44
  • @haithamhany Honestly, I didn't understand your question. What do you mean by "correct output but not the correct amount"? – Yeldar Kurmangaliyev Aug 17 '15 at 04:49
  • I mean when you try my simple example, which takes 10 3 1 2 3 2 3 1 1 1 1 3 the output is is 5 2 instead of 5 2 3 and so on – haitham hany Aug 17 '15 at 04:53
  • @haithamhany Oh, now I see. Updated the answer. The last output loop should be `for (int i = 0; i <= maxValue; i++)` but not `for (int i = 0; i < maxValue; i++)`. It didn't output an answer for the last `Max` value because of this typo :) – Yeldar Kurmangaliyev Aug 17 '15 at 04:59