Priting unique values in a array c++ my code work not for all the cases

Question

I have written a program that has to print the unique values in an array, but this source code seems not to be working correctly. I hope that someone can help me correct it.

#include <iostream>
using namespace std;

bool is_in(double vettore2[], int n, int k) {
    bool isin = true;
    int i;
    for (i = 0; i < k; i++) {
        if (vettore2[i] == n) {
            isin = true; // value already present in second array
        } else {
            isin = false; // value not present in second array
        }
    }
    return isin;
}

int main(void){
    double vettore[6] = {1,2,2,1,2,5};
    double vettore2[6];
    int i, k;
    vettore2[0] = vettore[0]; 
    k = 1; // k counts the number of elements in the second array
    for (i = 0; i < 6; i++) {
        if (!(is_in(vettore2, vettore[i], k))) {
            // if value is not in second array then insert it
            vettore2[k] = vettore[i];   
            k++;
        }
    }
    for(i = 0; i < k; i++) { // print second array   
        cout << vettore2[i];
    }
    return (0);
}

Congratulations on posting the vaguest problem statement of the day, possibly the week. — user657267, Oct 24 '14 at 07:33
Compile with all warnings and debug info (`g++ -Wall -Wextra -g -std=c++11`). Then use the debugger (`gdb`). Better code in C++11 — Basile Starynkevitch, Oct 24 '14 at 07:33
okk.. check this:: If your `vettore2` array is `1,2,3` and then the next element comes out to be `2` from `vettore` array, and the function which checks if the element is present in the array will still return false and insert `2`, in the array, which gives a wrong answer. You need to insert a `break` statement in the if condition. — user007, Oct 24 '14 at 07:41
Can you please tell what exactly is not working ? I am sorry I did not understand `this source code seems not to be working correctly` — Code Geek, Oct 24 '14 at 07:44
Please review how your posts are going to look in the preview before posting *(the preview updates as you enter your question--it's hard to miss!)* [Your original was unreadable with bad indents](http://stackoverflow.com/revisions/26543332/1), and as little spacing as you used in general, the random newlines just frustrate people. Also try to be strategic with your comments. Often a good variable or function name can remove the necessity of a comment! And long comments that cause horizontal scroll bars on code samples are not received well here. More looking before posting = less downvotes! — HostileFork says dont trust SE, Oct 24 '14 at 07:45
@user3503186 You could do the assignment simpler without using a second array. — Vlad from Moscow, Oct 24 '14 at 08:01
Vlad from Moscow you say that i can do the problem without the second array. how? — Computerlucaworld, Oct 24 '14 at 08:08

mehmetseckin · Answer 1 · 2014-10-24T08:36:47.213

1

Your loop is faulty you should break when you see a duplicate.

bool isin = false;
for(i=0;i<k;i++)
    if (vettore2[i]==n) {
        isin = true;
        break;
    }
return isin;

edited Oct 24 '14 at 08:36

answered Oct 24 '14 at 07:36

mehmetseckin

3,061
1
31
43

2

In my opinion it is a bad code. – Vlad from Moscow Oct 24 '14 at 08:00
Vlad from Moscow. Can you tell me how to improve my code – Computerlucaworld Oct 24 '14 at 08:26

score 0 · Answer 2 · answered Oct 24 '14 at 07:35

I suggest letting a set handle your problem.

http://www.cplusplus.com/reference/set/set/

You can use the insert method (http://www.cplusplus.com/reference/set/set/insert/) to insert your values into it, if the value already was inserted it will give a different return value, so you know that it is not unique.

It is often better to rely as much as possible on stable and well tested functions instead of writing everything from scratch.

score 0 · Answer 3 · edited May 23 '17 at 12:05

Although you've gotten your answer as to where "the bug" is, there are a lot of things to consider. Idiomatic C++ is more robust, short, and nicer to look at...even when dealing with linear searches.

Here's the equivalent of your fixed code using std::vector and std::find in C++11:

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

int main() {
    vector<double> vettore {1,2,2,1,2,5};

    vector<double> vettore2;
    vettore2.push_back(vettore[0]);

    for (auto & n : vettore) {
        if (find(begin(vettore2), end(vettore2), n) == end(vettore2))
            vettore2.push_back(n);
    }

    for (auto & n : vettore2)
        cout << n << " ";

    return 0;
}

(Note: I will point out that doing the initial insertion into vettore2 doesn't do you any good if you're going to check the first element of vettore anyway. I'd just leave that out.)

Not only is it nicer, but find is abstract. It doesn't care what kind of collection it is operating on...because it uses iterators it can work even if a collection can't be indexed by integer. The hardcoded number "6" is gone and the vectors will grow on demand if they need to get bigger. You don't need k because the vectors keep track of their own size. Etc. It is orders of magnitude better code because it leverages the language as it was designed to be used.

Note that you will get a confused with floating point "equality" if you do not read up on it:

What is the most effective way for float and double comparison?

@user3503186 For further reading on using algorithmic building blocks to solve similar problems, see: [Most efficient way to erase duplicates and sort a C++ vector](http://stackoverflow.com/questions/1041620/most-efficient-way-to-erase-duplicates-and-sort-a-c-vector) — HostileFork says dont trust SE, Oct 24 '14 at 08:28

Priting unique values in a array c++ my code work not for all the cases

3 Answers3