What's wrong with my code about find the max one and the min one by vector

Question

I wrote a code，it will find the max one and min one in an arr by vector but I simply don't know what I've made an error.

Every time I try it return zero for me.lol

#include<iostream>
#include<algorithm>
#include<vector>
using namespace std;
int main()
{
    int a[999];
    int n;
    cin >> n;
    for (int i = 1; i <= n; i++) {
        cin >> a[i]; 
    }
    vector<int> arr(a[1], a[n]);
    int max = *max_element(arr.begin(), arr.end());
    int min = *min_element(arr.begin(), arr.end());
    int dis = max - min; cout << dis << endl;
    return 0;
}

What?? `int a[999];` and then you put it in a vector, but you index everything starting at one? Please go and get a good C++ book! — Matthieu Brucher, Feb 08 '19 at 11:17
@jonhnnyChiu -- What would happen if I had the patience to enter more than 999 values? — PaulMcKenzie, Feb 08 '19 at 11:19
do yourself a favour and drop your current learning resource and take a look [here](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) — 463035818_is_not_an_ai, Feb 08 '19 at 11:20
`vector arr(a[1], a[n]);` this is just nonsense, you're passing two `ints` to the constructor. How is this supposed to make a `std::vector` out of the array? Please look up the constructors of `std::vector` here: https://en.cppreference.com/w/cpp/container/vector/vector — Blaze, Feb 08 '19 at 11:20
In addition to what already been said: Arrays in C++ are 0-based, not 1-based. The fact, that you ignored 0th element in your code, suggests to me, that you didn't know that. — Algirdas Preidžius, Feb 08 '19 at 11:26
@Blaze indeed, leading to an array with always the same value and so min = max and dis = 0. Really easy to spot with a debugger. — Matthieu Brucher, Feb 08 '19 at 11:27

463035818_is_not_an_ai · Accepted Answer · 2019-02-08T12:29:44.170

What's wrong with my code [?]

Honestly, almost everything.

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

using namespace std; can cause harm, not in small examples like this, but better don't get used to bad habits.

int main()
{
    int a[999];

You allocate space for 999 elements. What if the user enters 1000 for n ?

    int n;
    cin >> n;

Array indices start at 0 not at 1. Lets say the user enters 999 then you are still going out of bounds invoking undefined behaviour because a[999] is not an element of the array.

    for (int i = 1; i <= n; i++) {
        cin >> a[i]; 
    }

You are calling the std::vector constructor that sizes the vector to hold a[1] elements with value a[n]. This is not copying the elements from the array to the vector! All elements will have the same value, hence you always get 0 as the result.

    vector<int> arr(a[1], a[n]);
    int max = *max_element(arr.begin(), arr.end());
    int min = *min_element(arr.begin(), arr.end());

This is not really wrong, but written unnecessarily complicated. max_element returns an iterator and you can use eg. std::distance to get the distance between two iterators.

    int dis = max - min; cout << dis << endl;
    return 0;
}

You could write it like this

#include<iostream>
#include<algorithm>
#include<vector>
int main()
{
    std::vector<int> x;
    int n;
    std::cin >> n;
    x.resize(n);
    for (auto& e : x) std::cin >> e; 
    auto max = std::max_element(x.begin(), x.end());
    auto min = std::min_element(x.begin(), x.end());
    std::cout << std::distance(max,min) << "\n";
    return 0;
}

Live Demo

PS: Be careful with std::distance. Prior to C++11 it was undefined behaviour to call it with a second argument that cannot be reached from the first iterator passed by going forward. Also note that there is std::minmax_element that will give you both iterators from only traversing the vector once.

If you perceive a question as "please review my code for any problem" (as you seem to have done), why don't you flag it as too broad or as belonging elsewhere? Even the more focused question "Which problem in my code causes the misbehaviour?" should be closed for several reasons. — Yunnosch, Feb 08 '19 at 13:53
@Yunnosch sometimes I do weird things. I consider my votes as just one out of five that are necessary to put on hold, just because I do vote does not imply that the question will get closed eventually. It was still open after an hour, so I thought why not... — 463035818_is_not_an_ai, Feb 08 '19 at 14:10
I see. And I admit that I sometimes feel the same. It is just the way you answered, which really looks like a review more than an answer to the problem - which was not even asked for... — Yunnosch, Feb 08 '19 at 15:07
@Yunnosch there isnt even a question to begin with, so I took the freedom to interpret it as "What is wrong with my code?" and that is my answer. Imho an answer is incomplete when it does not point out obvious problems in OPs code even when it was not explicitly asked for — 463035818_is_not_an_ai, Feb 08 '19 at 15:13
True, but those parts should go after answering the (guessed) actual question. Just to avoid confusion, not only with OP. — Yunnosch, Feb 08 '19 at 15:14

What's wrong with my code about find the max one and the min one by vector

1 Answers1