0
\$\begingroup\$

The program reads in a queue of integers and returns determines whether or not it is valid (ascending). How can this code be improved? I assume iterator include statement is unnecessary?

#include <iostream>
#include <queue>
#include <deque>
#include <iterator>

template<typename T, typename Container=std::deque<T> >
class iterable_queue : public std::queue<T,Container>
{
public:
    typedef typename Container::iterator iterator;
    typedef typename Container::const_iterator const_iterator;

    iterator begin() { return this->c.begin(); }
    iterator end() { return this->c.end(); }
    const_iterator begin() const { return this->c.begin(); }
    const_iterator end() const { return this->c.end(); }
};

bool checkValidity(iterable_queue<int>& q) {
    if (q.empty() || q.size() <= 1){
        std::cout << "invalid entry, insufficient elements" << '\n';
        return false;
    }
    while(q.size()){
        auto i = q.begin();
        auto j = ++q.begin();
        for(; i < q.end() && j < ++q.end();){
            std::cout << *i << " " << *j << '\n';
            if (*(i) > *(j)) {
                std::cout << "invalid entry, not properly sorted" << '\n';
                return false;
            }
            i++, j++;
        }   
        std::cout << "valid entry, properly sorted" << '\n';
        return true;
    }
    std::cout << "invalid entry, insufficient elements" << '\n';
    return false;
}

const char* bool_cast(const bool b) {
    return b ? "true" : "false";
}        

int main () {
    iterable_queue<int> numbers;
    int temp;

    std::cout << "Pushing..." << '\n';
    while(temp >= 0){
        std::cout << "Enter numbers: ";
        std::cin >> temp;
        if(temp >= 0){
            numbers.push(temp);
        }
    }

    bool ck = checkValidity(numbers);
    std::cout << bool_cast(ck) << '\n';

    std::cout << "{ ";
    while(numbers.size() > 0){
        std::cout << numbers.front();
        numbers.pop();
        std::cout << " ";
    }
    std::cout << "}" << '\n';

    return 0;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ What would happen if q.size() yields zero? \$\endgroup\$ – πάντα ῥεῖ Apr 3 '20 at 14:53
  • \$\begingroup\$ Return false. invalid entry, insufficient elements. \$\endgroup\$ – Darnoc Eloc May 24 '20 at 3:12
1
\$\begingroup\$

Queues are not designed to be used like this. Your iterable_queue clearly shows that you are not using queues the way they are supposed to be used. And the checkValidity function should be simplified with std::is_sorted from the standard library. In q.empty() || q.size() <= 1, the first condition is subsumed by the second. The while(q.size()){ loop also seems to have no significance. ++q.end() is also undefined behavior. The bool_cast functionality is already provided by std::boolalpha.

Overall, I think you are seriously over-complicating everything:

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

int main()
{
    std::vector<int> numbers;
    for (int num; std::cin >> num && num >= 0;) {
        numbers.push_back(num);
    }

    bool valid = numbers.size() > 1 && std::is_sorted(numbers.begin(), numbers.end());
    std::cout << "Valid? " << std::boolalpha << valid << '\n';

    std::cout << "{ ";
    for (int num : numbers) {
        std::cout << num << ' ';
    }
    std::cout << "}\n";
}

(live demo)

\$\endgroup\$
2
  • \$\begingroup\$ The point of the exercise is to use the queue container and not a vector. \$\endgroup\$ – Darnoc Eloc Apr 4 '20 at 19:18
  • \$\begingroup\$ @DarnocEloc You are deriving from queue and accessing the underlying container directly. That's not how a queue is supposed to work, so in a sense, you are already disregarding "the point of the exercise." I guess the intention of the exercise is to validate the numbers when you read them or when you print them (because traversing a queue is destructive). Anyway, I don't think using queues is an "improvement" for the code, so I'll leave my remarks for other members of the community. \$\endgroup\$ – L. F. Apr 5 '20 at 2:10

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.