Skip to main content
Code Review

Return to Answer

Spelling, terminology and formatting improvements
Source Link
Toby Speight
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327

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 peekpeek() or dequeuedequeue() is called, and not in enqueueenqueue().

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

Avoid Using Namespace STD
If If you are coding professionally you probably should get out of the habit of using the "using namespace std;" statementusing namespace std; directive. The code will then more clearly define where cout and other functionsthe objects/functions are coming from (std::cinstd::cin, std::coutstd::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 functionobject cout you may override within your own classes. This stack overflow questionStack Overflow question discusses this in more detail.

Magic Numbers

Magic Numbers
Numeric Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. The values for the variable kk are defined by the problem, but it might be better to use symbolic constants rather than raw numbers in the switchswitch statement. That would make the code easier to read and maintain. C++ provides a couple of methods for this,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 stackoverflowon Stack Overflow.

Use Descriptive Variable Names

Use Descriptive Variable Names
The variables s1 The variable names s1 and s2s2 are not very clear, and if they weren't in a std::stackstd::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 and x. As an example, for k I might use queryIndexqueryIndex.

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

Prefer to Not Include What Isn't Necessary

Prefer to Not Include What Isn't Necessary
It It is much better to only include what is necessary in a source file. There are currently 6 filesheaders included, but only two are necessary (iostream<iostream> and stack<stack>) for this implementation. Including cstdio<cstdio> is bad because it might lead the programmer to use C I/O statementsfunctions 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>).

Prefer One Declaration Per Line

Prefer One Declaration Per Line
Maintaining 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.

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

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 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;" statement. The code will more clearly define where cout and other 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 function 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 stackoverflow.

Use Descriptive Variable Names
The variables s1 and s2 are not very clear, and if they weren't in a 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 files 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 statements 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>).

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.

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

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>).

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.

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

typo fix
Source Link
dfhwze
dfhwze
  • 14.2k
  • 3
  • 40
  • 101

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

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

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.

Source Link
pacmaninbw ♦
pacmaninbw ♦
  • 26.1k
  • 13
  • 47
  • 114

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

The rest of 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;" statement. The code will more clearly define where cout and other 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 function 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 stackoverflow.

Use Descriptive Variable Names
The variables s1 and s2 are not very clear, and if they weren't in a 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 files 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 statements 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;

Versus

    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;