65
namespace std
Others have said this already but it's important enough to be repeated: Don't put your own definitions into namespace std. It's undefined behavior.
About the only thing I'm aware of that you may put into namespace std are specializations for templates already defined by the standard. So, for example, if you have
struct MyType
{
int a;
};
...
40
Funny how the universe works - I just finished my own implementation of a thread pool (albeit in C++17), and it looks a lot like yours. I found this question on the front page when I went to post my own - here's hoping we're both on the right track!
Mark the copy c'tor and operator= as deleted
Instead of actually implementing something that you don't want to ...
38
Overview
The code is OK.
I found 1 bug.
The encapsulation is slightly lacking. You have Account as a class, which is good, but the getters/setters are not required and you have better methods to interact with it.
On encapsulation, you need to encapsulate a set of accounts and the "next account number" into a class (maybe Bank). Currently, you set up a ...
38
Why do most people (on the internet) recommend using recursion because it's simpler and easier to write the program? Logically I thought that we should write it in a way that is fast and simple.
This is a perceptive question. I wrote an article about exactly this topic in 2004, which you can read here:
https://docs.microsoft.com/en-us/archive/blogs/...
27
Don't recreate your random engine everytime
Here:
std::random_device randomDevice;
std::mt19937 engine(randomDevice());
std::uniform_int_distribution<int> distribution(minimumExperience, maximumExperience);
This does not make it more random and can affect the quality of numbers you get. Random engines have some ...
26
Here are some observations and suggestions that may help you improve your program.
Check your spelling
If you run a spell check on your comments, you'll find a number of things such as "peice" instead of "piece" and "accpets" instead of "accepts". Since your code is nicely commented, it's worth the extra step to eliminate spelling errors.
Separate ...
17
Style
You're not being charged by the character; there's no need to abbreviate "count", or "ret" (which I would call "score" instead). Also, main has inconsistent brackets with the rest of the program. Other than that, it looks good.
Globals
The globals are only being used by greed_rand, and would be better created within main and passed as parameters to ...
16
I believe I can help with these things:
Obliterate macros
Obliterate raw arrays
Macros can be replaced with std::integer_sequence (C++14), and raw arrays can be replaced with std::array (C++11).
#include <array>
#include <utility>
template <typename T, T... Is>
constexpr auto gen_primes_helper(std::integer_sequence<T, Is...>) {
...
16
This looks pretty good. I'd propose the following.
First, you use set_value in exactly one place. It's a pretty unnecessary function, and just this would suffice:
FlagSet &operator|=(const T &val)
{
bitset.set(static_cast<utype>(val));
return *this;
}
Second, for &=, the expression expr ? true : false is an antipattern that can ...
15
If you like new features, and even experimental features, you can make your code a lot cleaner.
Concepts
A lot of those arcane SFINAE techniques will be obsolete once concepts are out, and concepts are already available in an experimental state with the last versions of gcc (enable them with the -fconcepts option):
template <typename Container, ...
14
If I'd asked an interview question, and got this as an answer (at least as the first answer) I think my immediate reaction would be to consider it excessively clever.
To make much sense, along with the conditions you gave, I'd have had to specify that minimizing memory usage was absolutely crucial.
Under ordinary circumstances, I'd expect an answer ...
14
std::uint64_t is consistently misspelt throughout the code, and may be a poor choice anyway (since an exact 64-bit type need not be provided). It's better to use one of std::uint_fast64_t or std::uint_least64_t instead, for better portability. (Edit: as suggested by supercat in a comment, only the exact-width type will work correctly here, given the ...
14
@osuka_ and @anderas gave some very good advice. I just have a couple things I want to add:
Macros
#define MAX_THREADS std::thread::hardware_concurrency() - 1;
While it doesn't really make sense to do arithmetic on this, macros like this should be surrounded by parentheses so the order of operations works correctly. Without it you get weird stuff like (...
13
Loop invariants code motion
This is the name of an optimization performed by most compilers: when they detect code that actually does not depend on the state of the loop, they move it out of the loop. It's not as obvious as the usual invariants, but let's have a look at this line:
Unsigned res_i = (lhs_p_cpy & rhs_p_cpy) ^ lhs_i ^ rhs_i;
lhs_i and ...
13
You've received good style advice so far.
But your pool doesn't work. So let's try to address that.
1. 'resize' is both wrong and not needed.
As others mentioned, it's not thread safe. Making it so is exceedingly difficult. And in practice, you don't need to change the thread pool's size after after start.
2. You only use one thread at a time.
In ...
12
This is a bug:
else if( record.size() > hash_table.rbegin()->second.size() )
As implemented, insertion stops once your program encounters the longest line in the file.
For example, suppose the input contains:
2
abcdefghijklmop
a
ab
abc
abcd
Your program will output:
abcdefghijklmop
a
The fix is simple: Change rbegin to begin.
Another potential ...
12
Break out your type trait
You have this large amount of is_same checks in a couple places. Break it out in its own trait, and call it something like is_char:
template <class T> is_char : std::false_type { };
template <> is_char<wchar_t> : std::true_type { };
template <> is_char<char> : std::true_type { };
template <> ...
12
Namespace
#ifndef LINKEDLIST_H
#define LINKEDLIST_H
The term "linked list" is a common term. So I would expect somebody out there is already using the macro LINKEDLIST_H. When somebody uses that library and your library then you will get some funny interactions.
As a result I would make the include guards a little more unique. When I define header guards ...
12
Coding idiomatically
Remember that #pragma once isn't standard (unlike include guards).
If you only intend your class works on major compilers, you can use #pragma once (but care about defects and drawbacks)
If you wish your class works on all compilers, use both #pragma once and the include guards.
If you want your class to be 100% compatible with ...
11
Generally speaking, it is really well done, for several reasons: std::tuple often takes advantage of the empty base class optimization, which means that since you feed it lambdas, your class will often weigh almost nothing, and everything is correctly forwarded. The only things I see that could be improved are the following ones:
You could const-qualify ...
11
You could probably use std::remove_if instead of your algorithm. Take this example code:
std::vector<std::string> vec = { "0", "1", "5", "0", "3", "6", "0", "1" };
std::remove_if(std::begin(vec), std::end(vec), [](const std::string& val)
{
return val == "0";
});
for (auto&& a: vec)
{
std::cout << a << ' ';
}
I get the ...
11
Programming / detail review
Fan(Fan *peer__ = NULL);
(and some more occurrences)
Identifiers that contain a double underscore are reserved for the compiler/libary ("C++ implementation").
The NULL macro can be thought of as deprecated. Use the nullptr keyword instead. It is more typesafe.
static Fan * create(Fan *peer = NULL);
Since those pointers ...
11
I've actually implemented the same functionality in the past, so here are my comments.
Wrong behaviour?
It depends on what you can consider wrong. Consider running your own example and inputting a a. The 2nd a will be left in the stream buffer and will be automatically assigned to your 2nd variable. This clearly causes weird behaviour, but it is the same ...
11
namespace std
is reserved. Don't add stuff to it. Especially since there's no reason to in this case. Just put it in your own namespace.
unnecessary string usage
There isn't actually any reason to take the prompt as a std::string. You could have kept it as a character literal. Really, anything streamable is good enough, so this could just be:
template &...
11
I recommend putting your functions into a namespace.
for (auto i = begin + 1; i != end; ++i) {
By doing begin + 1, you require that Iterator is a RandomAccessIterator, whereas you only need an ForwardIterator. Use std::next instead:
for (auto i = std::next(begin); i != end; ++i) {
You could also use std::next(begin, 1) to be explicit.
Similarly, when ...
11
std::swap(val1, val0);
This hard codes std::swap as the function to use for swapping. C++ also has a powerful (and sometimes confusing) mechanism to allow multiple namespaces to be searched for a function: argument dependent lookup (ADL).
To make use of it call swap unqualified, which performs ADL to find swap functions in the namespaces of the arguments, ...
11
Speed
Of course we all know bubble sort is one of the slowest sorting algorithms1. It turns out, however, that stopping when an iteration is completed without a swap usually adds more overhead than it saves. I did a quick test with this code:
template<typename Iterator, typename Comparator>
void bs2(Iterator begin, Iterator end, Comparator cmp) {
...
11
I think std::rotate can lead to a somewhat simpler approach:
get the input
create a copy
rotate the copy by one element
Accumulate the sum of elements that match between the two
Code might look something on this general order:
int sum_matching(std::vector<int> const &digits) {
std::vector<int> copy{digits};
std::rotate(copy.begin()...
11
Warnings
It is usually a good idea to compile your code with all warnings enabled to see if anything is wrong. In your case, nothing really worrying but we may as well fix it.
Press ENTER or type command to continue
euler37.cpp: In function ‘std::vector<long long int> primesUpto(long long int)’:
euler37.cpp:16:26: warning: comparison between signed ...
11
Try not to using namespace std
It is considered a bad practice. This is because namespaces were introduced as a way to avoid name collisions, i.e Multiple objects with the same name. The std namespace is HUGE and has hundreds of common identifiers that can interfere with yours.
By doing using namespace std you now have no idea what function is a part of the ...
Only top voted, non community-wiki answers of a minimum length are eligible
