Hot answers tagged

25

Profiling: We could use a library like Tracy to do the sort of profiling you mention in the question. But it's quicker to ask "what's taking the most time" and get an answer with a profiler like Very Sleepy, or the built in Visual Studio profiler. So... I stuck your code into a project created with create-vs-napi, and ran stringify on some json ...


16

What further improvements can I make to this code? The code is rather bad - the site you read it from tends to have lots of bad/amateur articles. The graph class: std::map<int, bool> visited; std::map<int, std::list<int>> adj; This is extremely poor and inefficient graph implementation. std::map creates a new memory allocation for each ...


12

Correctness and testing: First off... note that compilers are not required to do anything with template code unless it's actually used ("instantiation"). Some compilers (or versions of a particular compiler) may check (some) template code for correctness, others may not. So writing a template function, we cannot rely on the compiler to give us a ...


12

Disagreement I have to disagree with @ALX23z blanket statement that using std::map is bad idea. There is a lot more to it than that (if the graph was sparse the map is great). But you don't give enough contect to evaluate the graph implementation. All I can say it is an acceptable "basic" implementation. Overall Things that are actually bad: 1: The ...


12

This is not a bad attempt at making a vector-like container, except it is lacking support for const and makes unnecessary copies (see below for a more detailed explaination). The variable names are quite well chosen. It's also good to see Doxygen documentation. Use std::size_t for the size of the storage The template parameter BUFFER_SIZE should have type ...


10

I've assumed here the OP is performing DFS on undirected graphs - if not only my second point is valid. There are several other answers here suggesting better coding practices and more efficient implementations, but they're missing the biggest problem with you code: it doesn't perform depth first search! To demonstrate this run it with the graph created by: ...


9

Answers template<typename InputIterator, typename = typename std::enable_if<!std::is_integral<InputIterator>::value>::type> vector(InputIterator first, InputIterator last) I can't understand this part: This is using SFINE to make that constructor available or not available depending on the type of the iterator used. If the user of your ...


9

Ephemeris::Ephemeris() { } No need for a constructor if it does nothing. Ephemeris::~Ephemeris() { if (telnet != nullptr) delete telnet; } The test here is pointless, since delete works fine with a null argument. We can improve on this by making telnet a smart pointer (probably std::unique_ptr<TelnetClient>), so that a default ...


9

Here are some things that may help you improve your code. Understand the nature of templates In modern C++, putting headers in a header file and the implementation in a .cpp file is common and ingrained. However, in this case, it's also not correct. The reason is that templated functions don't actually cause the compiler to generate any code until and ...


8

T internal_buffer[BUFFER_SIZE]; This isn't what you want for types that are expensive to construct, or can't be default-constructed. Your interviewer probably wants to see use of an uninitialized memory buffer, with placement-new and explicit destructor call to create and erase values. When you get to C++17 or later, then you should probably be using std::...


7

Debug-code The proper destination for debug-output is std::clog (which goes to STDERR by default, std::cerr has the same destination but no buffering), not std::cout (which goes to STDOUT). Doubly so for trace-output. I guess you resort to std::endl because you use the wrong stream, though that's still a poor excuse. If you need an explicit flush, be ...


7

This isn't bad, but it's not stellar either. Names matter. Stable is a very strange name here. It evokes pointer/iterator stability, a term used to indicate that pointers and iterators are stable in memory, for an construct in which they will NOT be. I would recommend using Inline, instead, to indicate that the memory used is right here (in line) rather than ...


6

Iterators don't need to be friends with class vector The iterator classes do not need access to private members of vector, so the friend declarations can be removed. Mistakes when using *this There are bugs in your implementation, mainly because you do things like: BDIterator operator++(int) { BDIterator res(*this); ++(*this); return res; } The ...


6

General stuff The implementation as-is does not attempt to be thread-safe in any way. I guess this is intentional, but might cause problem and/or confusion if not communicated well. There is no std::weak_ptr-like analogue. Might be a new challenge to implement later ;) Move constructor (and assignment operator) would be nice for SharedPtr. I was first ...


6

X(); Since we specify a value constructor, the compiler won't generate the default constructor. So we don't need to declare it private to prevent its use. The move operators can declared = default: X(X&& moveFrom) = default; X& operator=(X&& moveFrom) = default; You actually appear to have two copy assignment operators instead ...


6

Looks pretty good; these comments are mainly nitpicking: Error messages should be streamed to std::cerr, not std::cout. Whilst that's correctly done in main(), we're using the wrong stream in parseDuration(). I'm not a fan of both modifying the dur parameter and returning it. I would go for accepting a default by value and returning the parsed/default ...


6

Avoid defining your own constants There's no need to use your own constants for the duration of a millisecond, second, minute and so on; just use the right std::chrono types: static const std::unordered_map<std::string, std::chrono::milliseconds> suffix { {"ms", std::chrono::milliseconds{1}}, {"s", std::chrono::seconds{1}}, ...


6

You are implementing std::span What you are trying to implement is basically C++20's std::span. It might be worthwhile to look at how it is implemented, and copy its interface if possible, so if you ever want to change the codebase using your class to use std::span, it will be easy. I'll also use std::span as a reference and point out differences between it ...


5

It's not clear to me why we need to downgrade from std::random_device to a timestamp-based seed on Intel platforms. If we have good reason to distrust a particular library's implementation, then submit a patch mixing timestamp into the randomness, rather than ignoring the platform's randomness. C++17 was standardised after this code was written; it provides ...


5

std::map<int, bool> visited; This is useless as a member variable. It means nothing before or after you call DFS, so it's a waste of space most of the time There's only one copy of it for a given Graph, which means you can't call DFS twice concurrently It retains its state after calling DFS the first time, which means you can't even call DFS twice in ...


5

use a function, not a class A CrcCal class instance has no state (non-static member variables). The class therefore has no reason to exist. We can implement the algorithm as a simple function that operates on a range of data, and has some additional parameters, e.g.: template<class T, T polynomial, T initial_remainder, T final_xor_value, bool reflect_data,...


4

Looks pretty good at first glance! I'd say your main problem is "#define ONE 1" — you've gone out of your way to parameterize things that can't actually be changed in real life without massive changes elsewhere in the game logic. For example, you've set enum Items { Rock, Paper, Scissors, ItemsCount }; which looks awesome... until someone decides ...


4

I agree with everything Toby Speight wrote. I'd like to add: Name things according to what they represent You have this line of code: tcp::resolver::iterator iterator = resolver.resolve(query); But while iterator is indeed an iterator, that's not what is important about this variable. What really is important is that it is the result of the DNS query. So I ...


4

if(0u < used_elements) This is confusing to read and takes more time to understand than if(used_elements > 0u) I can understand using the reversed equality comparison 0u == used_elements to prevent accidentally writing used_elements = 0u (although I personally don't like that), but this is entirely unnecessary for inequalities.


4

Documentation Add documentation. I had no idea what the goal of this program is, going in, and at the end of the review I still don't, so I can't comment on whether the code is correct or efficient. Do we want to output the most likely child for a father? The most likely father for a child? What does "singleParent" mean? Often in this context it ...


4

Design review Your design is mostly sound. It can be simplified a little bit, and optionally made a bit more rigorous, but the basic design is not bad. The name for what you’re doing is the “Policy” design pattern. The gist is that you create a class templated on policy types, and then concrete policy classes to control the behaviour of the main class. Even ...


3

Your ConditionalOStream is not a std::ostream: The advantage is that formatting will be completely skipped if it doesn't have a target. The disadvantage is that all functions expecting a std::ostream must be modified to accept this new type. Just writing a blackhole-streambuffer reverses the picture: You have the standard std::ostream-interface. But even if ...


3

Code Review The main purpose of the shared pointer is to prevent the leakage of pointers. Thus as soon as you hand the pointer to the shared pointer object, it becomes responsible for it and you must ensure that it is deleted. Here in the constructor that means you must protect against failures in the constructor. Note: If the constructor fails to complete, ...


3

Review The code is well formatted and also reasonably well implemented, lacking only a few security overhauls such as memory leaks and optimizations also completing the pending implementations of existing concepts in unique_ptr Is it well-formed? Yes, have good formatting i.e. does it follow common C++ standard and patterns (for example, should private ...


3

Some things to think about: different PRNG algorithms have different trade-offs. The MT19937 algorithm is very fast, but it has a huge internal state (19937 bits to be exact, which is how it got its name). While this is a trivial amount of memory on desktop and server class CPUs, consider that on devices with less (cache) memory, this can be a significant ...


Only top voted, non community-wiki answers of a minimum length are eligible