26

Do not use using namespace std;, instead explicitly use std:: where required. [Why is “using namespace std;” considered bad practice?] Consider not using prototypes (which requires you to maintain the same function header in two places) unless necessary. This means putting main() at the bottom of your code and functions that it uses above it. Comments such ...


24

Loki's solution does not enforce the order in which the function calls are performed, because the order in which function arguments are evaluated is unspecified. Here's a C++14 solution that ensures the function is called from left to right: #include <cstddef> #include <tuple> #include <utility> template <typename Tuple, typename F, ...


23

Readability Please put a space after the include and before the < #include<iostream> #include<string> #include<memory> Don't do this You can read any other C++ revue on this site. using namespace std; See: Why is “using namespace std” considered bad practice? Don't add useless comments // Template Node template <typename T> ...


22

Design The main thing about a vector is not constructing its members until they are put into the container. The vector you have constructs all the members immediately which can be very expensive if your type T has an expensive constructor (or you never use any of the members). T* storage = nullptr; As a result you don't want to use T* as your storage type ...


21

... as I went through others, there is always this voice in head, which says: "Oh, this is a lot of code and functionality. Is that really necessary or would it slow down the performance?" This is a reasonable consideration, but premature optimisation is evil for a reason. It might be a good idea to try to roll out your own solution, but it would be ...


18

I would have probably applied the following changes: Make size a static constexpr variable in CallerTraits instead of simply static const. Wherever a function simply passes variadic arguments whose types have been deduced, I would have passed args by universal reference (now officially called forwarding reference) and used std::forward to forward the ...


17

I find your implementation a bit more complex than necessary. What you want to do is fetch arguments from your "result set" row by calling its getValue() in a particular order; use them (as arguments) to call operator() on function object action. This can be done without recursion in two lines: Do{row->getValue(std::get<N>(args))...}; action(std::...


16

File extensions Separating the interface from the implementation is great. However, the .h/.hpp split is not obvious. The most frequent file extensions used for implementation files that cannot go into .cpp files are .inl (for inline) and .tpp (for template cpp). I know that at least some code editors or IDEs recognize the extension .inl (at least Code::...


15

Don't flush where you don't need to. Flushing is expensive, so don't use std::endl unless you need it. Nearly always, the shorter '\n' suffices, which can be merged with any bordering string-literal. Still, that advice seems to be for your teacher, not you ;-) return 0; is implicit in main if control reaches the closing brace }. The same is true for C ...


15

Use input redirection instead of cat | ... Instead of cat somefile | somecommand, you should use input redirection, for example: xgengen bash < Samples/template_bash_CMakeLists.txt | bash - > CMakeLists.txt The advantage of this is that you're running one fewer process (no more cat). Use modern style command substitution $(...) Don't use old style ...


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

I understand that some of this class is predicated by your professor; However I will review the code as a whole and you can then choose what can be changed and what cannot be changed. Interface/High-level comments Use of leading underscore _ has severe limitations. The C and C++ standards reserve the use of certain patterns of leading underscores for ...


14

Yes, there is. In fact, a very nice way to solve this code redundancy issue comes to mind if you consider that std::string is a just type alias for std::basic_string<char> and std::wstring an alias for std::basic_string<wchar_t>. Likewise, std::stringstream and std::wstringstream are also just aliases for std::basic_stringstream<char> and ...


13

You said that you packed all the suggestions from the previous question, but there are still some pieces of advice that you did not integrate into your code. Here is how you can still improve your it: math.h legacy In your code, you are using the constant M_PI. While it will probably work on most of the compilers I know, this macro isn't defined anywhere ...


13

There is a rather large amount of code here and a lot of macro trickery, so I shall refrain from attempting to comment on it all. There are, however, a few things that immediately jumped out at me. Don't use new as your allocating macro. That's extremely confusing, and it's going to make compiling it as C++ rather difficult. (Not that having C be ...


13

Your code is simple and just works. For that reason, you can be happy and there is not much comment to give. However, being very picky just for the sake of learning, one could say various things. print statements You are using print without parenthesis. This is the one and only reason why your code wouldn't work on Python 3. Just to be future (well, ...


12

Bottom line: your code here is focused and solid. It's a well-written introductory use of templates. However I'm not sure it's a great scenario in which to use them. First I'll talk about ways to improve its reusability, and then I'll say more about why I think it's a questionable scenario. As you start trying to implement your own templatized data ...


12

Below is how I would clean this up, or maybe partially re-write [live example]: // helpers template <typename T> struct id { using type = T; }; template <typename T> using type_of = typename T::type; template <size_t... N> struct sizes : id <sizes <N...> > { }; // choose N-th element in list <T...> template <size_t ...


12

[...]I decided that it would be fine to have something like an arbitrary-keyed map, that would require the key to be stored in the object (which allows to use the object's name as a key for instance). To restate your problem: you want the add behavior of a set (i.e., you add the object to the collection without any other information), but you want the ...


12

If you provide a destructor you should handle copying and assigning. In other words follow the Rule of 3 (or 5 if you care about move semantics). You can also disallow them but then there should be move constructor and assignment. Otherwise you will get in trouble with double freeing when calling a function void foo(Stack<int> s). empty() doesn't ...


11

I might have a number of minor stylistic comments but this is not important. What is important is the desired function decorate(): template <typename S> using result = typename std::result_of <S>::type; template <template <typename> class E, template <typename> class... D> struct decorate_impl { template <typename A, ...


11

I would appreciate all criticism relevant to code, style, flow, camelCase vs underscore, and so forth. First, (contrary to Loki Astari's answer) I think your style is correct (i.e. please do not capitalize the first letter of your classes - keep them matching the std:: style). Regarding the APIs of your code: Your code doesn't enforce const correctness ...


11

There's a lot of code here, so I'm just going to look at the documentation, the _split_by_braces function, and the visit_Str method. You'll see that there's more than enough for one answer. Maybe some of the other users here will comment on the other parts of your program. 1. Documentation Most submissions to Code Review have no documentation at all (...


11

#include <iostream> You don't seem to need this include. size_t _width; size_t _height; These should both be std::size_t. CSV(const std::string& filepath) Single argument constructors should almost always be explicit. if (!file) throw std::exception(); What you want to throw here is std::runtime_error or something derived from it, not ...


10

SetUtilites is misspelled; it should be SetUtilities. For better readability, consider doing something about this long line: static boost::unordered_set<Type> intersection(const boost::unordered_set<Type> &set1, const boost::unordered_set<Type> &set2) You could shorten the types as such via typedef, but it may not help very much, ...


10

I would add: do not open the ofstream in writeToFile. Have writeToFile take an std::ostream& parameter instead of the file path. You can then try to open the file early (and fail fast if case be), instead of processing everything and finally failing because you can't create the file. the fileType parameter in buildCOrCPlusPlusFile is a gratuitous change ...


10

In addition to the previous good answer, in order to keep your code efficient not only for the C++ primitive data types but also for the compound user defined types, I suggest: Make the member functions 'constructors', setValue1, setValue2, all take their arguments as const&. For instance: template <typename T1, typename T2> void Calculator<T1,...


10

If you have a destructor you should also define the copy constructor and copy assign + the move variants. Otherwise the compiler will generate its own (incorrect) copy and move facilities which will lead to dangling pointers and double frees. When pushing the element is copied. You should also provide a move variant of push: template<class T> void ...


10

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


10

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


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