3
\$\begingroup\$

This is still my first week of learning C++, I solved the lower bound STL challenge in Hackerrank Lower bound STL but I'm having time exceeded issue for long input. I'm open to constructive criticism, ways to improve in terms of syntax, style and performance.

You are given \$N\$ integers in the sorted order. Then you are given \$Q\$ queries. In each query, you will be given an integer and you have to tell whether that integer is present in the array if so you have to tell at which index it is present and if it is not present you have to tell the index at which the smallest integer that is just greater than the given number is present. Lower bound is a function that can be used with a sorted vector.

Input Format

The first line of the input contains the number of integers \$N\$ . The next line contains \$N\$ integers in sorted order. The next line contains \$Q\$ , the number of queries. Then \$N\$ lines follow each containing a single integer \$Y\$ . If the same number is present multiple times, you have to print the first index at which it occurs. The input is such that you always have an answer for each query.

Constraints

  • \$1 \leq N \leq 10{5}\$
  • \$1 \leq X_i \leq 10{9}\$ ,where \$x_i\$ is i{th} element in the array
  • \$1 \leq Q \leq 10{5}\$
  • \$1 \leq Y \leq 10{9}\$

Output Format

For each query you have to print "Yes"(without the quotes)if the number is present and at which index it is present separated by a space. If the number is not present you have to print "No"(without the quotes) followed by the index of the next smallest number just greater than that number. You have to output each query in a new line.

Sample Input

 8
 1 1 2 2 6 9 9 15
 4
 1
 4
 9
 15

Sample Output

 Yes 1
 No 5
 Yes 6
 Yes 8

Here is my code

#include <cmath>
#include <cstdio>
#include <vector>
#include <iostream>
#include <algorithm>



int main() {
    /* Enter your code here. Read input from STDIN. Print output to STDOUT */  
    int N;
    int query;
    int queryLength;
    {

    std::vector<int>::iterator low;
    std::vector<int>::iterator  foundValue;
    std::cin >> N;

    std::vector<int> v(N);
    //populate the vector
     for(int i =0; i<N; i++){
         std::cin >> v[i];
     }

    std::cin >> queryLength;

    for(int j = 0; j < queryLength; j++){
         std::cin >> query;
         foundValue= find(v.begin(), v.end(), query);
         if (foundValue != v.end())
         {
             std::cout << "Yes "<< (foundValue-v.begin()+ 1) << std::endl;
         }
          else{
            low = std::lower_bound (v.begin(), v.end(),query);
             std::cout << "No "<< find(v.begin(), v.end(), *low)-v.begin()+ 1 << std::endl;
          }
    }
}
    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Why use find and lower_bound. find() is O(n) while lower_bound() is O(ln(n)). You can use std::distance() to find the index from two iterators. \$\endgroup\$ – Martin York Nov 13 '16 at 6:13
4
\$\begingroup\$

Although you've probably already solved this problem, I'll still post my review for completion's sake.

1. Performance

Just as Loki said, you should use std::lower_bound() and std::distance() to solve this task, because std::find() has higher complexity.

2. General tips

  1. Do not include unused libraries (in this case, <cmath> and <cstdio>).

  2. Only declare variables when you need them. Declaring all the variables at the beginning of the program is old C style practice, which is discouraged in C++.

  3. Prefer prefix to postfix operator++. The postfix version creates a copy, so if you do not need that copy, you should use the prefix version.

  4. If you have access to C++11, you should consider using auto more. It'll make your code easier to read and save you some typing.

  5. You can safely omit return 0 from main();

[C++11: 3.6.1/5]: A return statement in main has the effect of leaving the main function (destroying any objects with automatic storage duration) and calling std::exit with the return value as the argument. If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;.

  1. Give your variables better names. Although other variable names in your code are good, a vector named v is not very descriptive.

  2. Use const_iterators for functions that do not alter the original range.

3. Final code

#include <vector>
#include <iostream>
#include <algorithm>

int main() {
    int N;
    std::cin >> N;

    std::vector<int> numbers(N);
    //populate the vector
     for(int i = 0; i < N; ++i) {
         std::cin >> numbers[i];
     }

    int queryLength;
    std::cin >> queryLength;    

    for(int j = 0; j < queryLength; ++j) {
        int query;
        std::cin >> query;
        auto lowerBoundIt = std::lower_bound(numbers.cbegin(), numbers.cend(), query);
        if(*lowerBoundIt == query) {
            std::cout << "Yes ";
        }
        else {
            std::cout << "No ";
        }
        std::cout << std::distance(numbers.cbegin(), lowerBoundIt) + 1 << '\n';
    }
}
\$\endgroup\$
  • \$\begingroup\$ A further improvement could be to use the previously-found result to narrow the search to one side or the other - the gain is probably not worth the complexity, though. \$\endgroup\$ – Toby Speight Sep 13 '18 at 10:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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