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


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


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


16

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


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


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

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


13

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


13

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


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

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


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

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


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


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


11

Design The problem with your implementation of pop() is that it can not be implemented in an exception safe manner (for any type of T). This is why the standard implementation implements this as two different methods top() and pop(). The top() function simply returns the value while pop() does not return the value but simply removes the value from head. So ...


10

Although there are definitely (at times annoying) limitations on what they can do, at first glance it looks like they don't apply here so I'd at least attempt to use a namespace alias: #pragma once #if __has_include(<optional>) # include <optional> using namespace opt = std; #elif __has_include(<experimental/optional>) # include ...


10

Is my code thread safe? If not, how do I make it thread safe? No, of course it's not thread-safe. You don't do anything to make it thread-safe. A more nuanced answer would be: It's thread-safe as long as you don't use it in an unsafe way. For example, calling Logger::log(FATAL, "hello world") from two different threads concurrently would of course be ...


10

You have some good answers already. My criticism comes from one word. "printf". The problem for fault-finding is not just knowing what each part has reported, but also knowing in what order they happened. Any practical logging library is going to hit threads sooner or later, and at that point things go wrong. For starters, we need to think about thread-...


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