Hot answers tagged

34

It's a good start! Here are some things that may help you improve your program. Don't abuse using namespace std Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Know when to use it and when not to (as when writing include headers). In this particular case, I happen to think it's perfectly appropriate ...


26

Lots of good code comments already given. I'll focus on non-code aspects. Take this from a DevOps engineer that regularly troubleshoots complex, unfamiliar systems from logs under time pressure. You want all logs to always be consistently formatted and to have timestamp and location (source file and line) by default, it looks like you request the date with ...


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


20

I see a number of things that may help you improve your code. Separate interface from implementation The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file. In this case virtually everything in datetime.h and all static functions in ...


20

We use std::string without including <string> - a possible portability bug. Representation as a string of char isn't very compact, so we're quite wasteful of space for large numbers. The modifying operators (++, *=, etc) all return void, but it's normal and expected that they return a reference to *this (act like the integers). It's suspect to have ...


19

Try to be more consistent with naming I'm seeing camelCase, PascalCase and snake_case all mixed together. Pick one style and stick with it. Furthermore, I see redundant things in names like Data_type_T, inconsistent use of the underscore suffix for private member variables, sometimes even using an underscore prefix which you should avoid. I would suggest ...


18

Prefer C++ headers (such as <cstdio>) to the C compatibility headers (<stdio.h>). The C++ headers define identifiers in the std namespace where we want them. It seems that this header isn't even used here, so we can omit it completely. std::size_t is misspellt throughout as size_t. This commonly happens when writing on a platform that declares ...


17

Unrestricted public getters and setters defeat the privacy of numerator and denominator. I recommend dropping getters and setters altogether, and let the class methods access the data fields directly. The constructor should call reduce. This way the fraction is always kept in the reduced form, and the operator== can be simplified to bool operator==(const ...


17

template <typename Less, typename T, typename... Ts> constexpr const T& min(Less less, const T& a, const T& b, const Ts&... rems) { This function requires a minimum of 2 elements. A minimum element will exist if the user provides a single argument variadic list. Consider handling that. auto& min1 = min(std::less<>{}, 4, 5); ...


16

Easy Stuff you already know. Probably part of your automated scripts to build new files. #pragma once But for new comers I would point out the more standard include guards are compatible everywhere. Don't be lazy std::lock_guard<xtd::fast_recursive_mutex> _(m_mutex); Though not technically wrong as an identifier (_). How many people do ...


16

Let me collect a couple of thoughts here. Aggregate initialization currently works like this: array2d<int, 2, 2> a{{1, 2, 3, 4}}; but wouldn't it be favorable to allow for array2d<int, 2, 2> a{{1, 2}, {3, 4}}; std::array::at performs bound checking and throws upon an out of bounds index. When your intention is to stick with the std::array ...


15

An idiomatic approach to implement this instead of basic arrays and raw pointers is to use iterators. Returning bool is dubious. The situation where I only want to know if the element is present or not is very rare. Typically I want to know where exactly the element is (or, if absent, where it should be inserted to keep the collection sorted). Your ...


15

Avoid excessive memory usage At each step of the recursion, you allocate two arrays that together are as large as the input. That means that your algorithm uses \$\mathcal{O}(N \log N)\$ space. It should be possible to rewrite your code so you only use \$\mathcal{O}(N)\$ space without substantially changing the algorithm. But this also brings me to: In-place ...


14

Okay so first of the obligatory Singletons are bad practice so you probably shouldn't make it easy to write bad code. Ignoring the fact that the class probably shouldn't exist at all we can look at the code. static T& GetInstance() { static MemGuard g; // clean up on program end if (!m_instance) { m_instance = new T(); } return *...


14

Singletons make it hard to test your code, and in my job I'd reject this at review for encouraging the development of untestable features. That said, I'll continue reviewing despite that. No need for helper class The MemGuard appears to be a poor man's reimplementation of std::unique_ptr. It would be much simpler for you to declare m_instance as a std::...


14

You're using cout without namespace qualifier which should not work. Personally I think typing std::string is better than importing the namespace part for it as it makes it clear which implementation is being used. Considering this is just a small showcase it might be debatable but I don't think pulling in boost just for a loop is a good idea. It ...


14

It looks pretty good, but I think there are some small improvements to be made here. Spell out namespace The using std::string; isn't really particularly useful here. I'd simply spell out std::string in the two places it's used. Fix the bug I'm sure you meant to write std::cout rather than just cout unless you've used using namespace std; which I'd ...


14

If you are just doing a single-shot printing of a container, just print the first element and then print delim-value pairs. This reduces the iterator requirements to being equality comparable, dereferenceable, and incrementable. if (!vec.empty()) { // invert to return immediately if this is a function auto first = vec.cbegin(); std::cout << *...


14

Making functions in header files static means that each translation unit gets its own separate definition. This usually isn't what we want. Standard practice is to declare the function in the header, and then define it in a .cpp file, e.g.: // header: #include <string> namespace lwlog { namespace datetime { std::string ...


14

Your magnitudes should be chou and kei (as in the supercomputer, as an aside). Also, to_Japanese() would be better named to_romaji(). As an exercise, you could try to_Japanese(number, KANJI|HIRAGANA|ROMAJI) also. Furthermore, it's icchou, not ichichou, and ikkei, not ichikei.


13

A modification of @Jerry's answer: #pragma once #if __has_include(<optional>) # include <optional> namespace stdx { using namespace ::std; } #elif __has_include(<experimental/optional>) # include <experimental/optional> namespace stdx { using namespace ::std; using namespace ::std::experimental; } #else # error <...


13

However, my goal here was to create a low-level data structure that is as time and space-efficient as possible. You don’t realize that unique_ptr is a zero-overhead abstraction. It does not add any size beyond the pointer being wrapped, and it does nothing but make sure you don’t call certain things, and handles the destructor which is code you need to ...


13

Although it's a short function/macro, there are a number of problems here: __FILE__ and __LINE__ are expanded in the function definition, rather than at the call site. We need an interface that passes those in, so that the macro is #define debug(args...) if (DEBUG) _debug(__FILE__, __LINE__, args); That's a non-standard variadic macro expansion. The ...


13

Using a power-of-10 base for extended precision is unusual, and only a good choice for specific use-cases (like if you mostly want to convert to decimal strings, or if multiplying / dividing by 10 is a major part of your workload). Choosing 10^1 specifically wastes more than half the bits in a char, and leads to a huge amount of operations for large numbers....


12

Refactor the limits There's quite a lot of repetition of std::numeric_limits<T>::max() and std::numeric_limits<T>::min(). It would make sense to define static const T min_val = std::numeric_limits<T>::min(); static const T max_val = std::numeric_limits<T>::max(); Or, we could make those limits be template parameters instead, which ...


12

I agree with you that 'printf-style' formatting was by many aspects better than C++'s manipulators: more concise, more varied, etc. That said, I feel like it's a step backwards if we port them into C++ without upgrading the C logic behind it. In my opinion, your code fails to do that for two reasons: lack of automatic memory management, and lack of type ...


12

This looks nice! Two issues I see here, When compiling your template with clang, it refuses the if constexpr dispatch for every recursive instantiation with sizeof...(rems) > 1, e.g. error: constexpr if condition evaluates to 2, which cannot be narrowed to type bool [-Wc++11-narrowing] gcc seems to accept this, but the fix is quite simple, just be ...


12

#include <iostream> #include <optional> #include <string> #include <string_view> #include <vector> #include <utility> #include <exception> #include <fstream> #include <cctype> #include <algorithm> I find it helps if I keep my includes in alphabetical order - that makes it easy to quickly check ...


11

Don't include <iostream> just to declare types Your header can be leaner if it replaces the big include with #include <iosfwd> which is provided for exactly this purpose. You will then need to include <iostream> in your implementation file, of course. Think about default values and implicit conversions Look at this: int numerator{...


11

Encouraging ineffective/wrong usage May be for your usage case it is important, but casts are usually an indication that something could be improved or fixed. static_casts are usually implicit. dynamic_casts are somewhat arguable, but there should be some better solution, albeit harder to find. const_casts are outright wrong (there is a case when non-const ...


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