an clas”

Posted on


You might want to look at the discussion tab for why the code is timing out. Basically the code should reverse the input stack only when peek() or dequeue() is called, and not in enqueue().

The rest of this answer is a review of the code as posted and it ignores the fact that HackerRank supplied some of the code, such as the includes and the using namespace std. Issues such as readability and maintainability are beyond the scope of HackerRank, but are considerations when writing good code.

Avoid Using Namespace std

If you are coding professionally you probably should get out of the habit of using the using namespace std; directive. The code will then more clearly define where the objects/functions are coming from (std::cin, std::cout). As you start using namespaces in your code, it is better to identify where each function comes from, because there may be function name collisions from different namespaces. The object cout you may override within your own classes. This Stack Overflow question discusses this in more detail.

Magic Numbers

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them.
The values for the variable k are defined by the problem, but it might be better to use symbolic constants rather than raw numbers in the switch statement. That would make the code easier to read and maintain. C++ provides a couple of methods for this; there could be an enum, or they could be defined as constants using const or constexpr. Any of these would make the code more readable. There is a discussion of this on Stack Overflow.

Use Descriptive Variable Names

The variable names s1 and s2 are not very clear, and if they weren’t in std::stack declarations I really would have no idea what they were. Since this is a queue problem it might be better to name them front and rear to represent what they are used for. It is very hard to maintain code with variable names such as s1, s2, q, t, k and x. As an example, for k I might use queryIndex.

Since the restrictions on all input indicates there will be no negative numbers, it might be better to use unsigned rather than int.

Prefer to Not Include What Isn’t Necessary

It is much better to only include what is necessary in a source file. There are currently 6 headers included, but only two are necessary (<iostream> and <stack>) for this implementation. Including <cstdio> is bad because it might lead the programmer to use C I/O functions rather the C++ ones. Including only what is needed improves compile times, because the contents of all the includes are part of the compilation. It could lead to other problems if you are implementing your own class rather than using a C++ container class (such as std::queue from #include <queue>).

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

Prefer One Declaration Per Line

Maintaining code is easier when one can find the declarations for variables. It would be easier to find where s2 is declared if it was on a separate line.

    std::stack<int> s1, s2;


    std::stack<int> s1;
    std::stack<int> s2;

Remember you may win a lottery and may not be the one maintaining the code.

The same reasoning applies to 2 statements on a line, such as

    int k; cin>>k;


One thing not already mentioned in the answers: When you have a member function which does not change the state of the object, then make it const:

// no
void peek(){ cout<<<<"n"; }

// better
void peek() const { cout<<<<"n"; }

// even better
int peek() const { return; }



Leave a Reply

Your email address will not be published. Required fields are marked *