Hot answers tagged

16

There are a number of 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. Use the required #includes The code uses a number of #includes that are not listed. It was not difficult to infer, but it helps reviewers if the code is ...


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


12

Exception safety Your locked_queue isn't exception safe. In particular: queue.pop(); return value; If the copy (or move) constructor for _T throws, you could have popped the item from the queue, then the constructor throws as you return the value, and the value is lost and can't be recovered. This is exactly why the standard library separates ...


11

First and biggest point: I really dislike spreading mutex locking throughout the code if it can be avoided. The first (and biggest) thing I'd change would be to create a concurrent_stack that handles the locking and unlocking internally. template <class T> class concurrent_stack { std::stack<T> data; Boost::mutex guard; public: ...


11

Your variables and functions should either be in camelCase or another naming convention that differentiates them from user-defined types (such as Game here), which are capitalized. If a parameter is not supposed to be modified, and it's not a primitive type (such as an int), pass it by const-reference in order to avoid an unnecessary copy. In both ...


11

You're essentially copying copy_if with some additional debug messages. One can rewrite your code with std::copy_if(all_items.begin(), all_items.end(), std::back_inserter(filter_items), [&bad_ids](const mystruct& item) { return std::find(bad_ids.begin(), bad_ids.end(), item.id) == bad_ids.end(); }); But that's not necessarily more ...


11

Well, lets start from the CodeReview: Since this is not really a library, I will omit my typical "Make it easy to use correctly, hard to use incorrectly", but it is still important thing to keep in mind. Using classes instead of structs In this case, it seems like struct should be used: struct point { int x; int y; double z; }; ...


10

It seems to me that if you want to insert an object into a stream, the "right" way to do it is normally to define an operator<< for that type. That goes for strongly typed enums just as much as it does for class/struct objects. #include <iostream> #include <vector> #include <utility> #include <iterator> enum class colors { red,...


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

First impressions I enjoyed reading this code. It's logically laid out (even the includes are alphabetical, so I can check at a glance), and I never felt like I was lost in the code. I do prefer my enum declarations to have one value per line - this can make it much easier to read diffs in future. Talking of enums, I'm not convinced there's much benefit to ...


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


8

Bug When your sets are of equal size you well end up in an endless recursion. Instead of if(set1.size() < set2.size()) you should use if(set1.size() <= set2.size()) (after all it does not make much sense to swap if the sets are of equal size). Use C++11 loops ... With C++11 I would advise you to use range based for: for(auto const &element: ...


8

There's a few things that jump out at me both in your implementation and standard options you missed. A few (scattered) thoughts follow. Returning a default if you care about error checking doesn't make sense. How would you determine if conversion failed if there's not an integer available for use as a sentinel? You are basically doing the same thing atoi ...


8

Be mindful of future enhancements (code always gets more complex over time) and localization. Your method assumes a very narrow range of future enhancements. You'll have trouble in the following possible enhancements: Print "Boom" for multiples of seven unless they are also multiples of 10 Print "Oy!" for prime numbers greater than 50 For any twin primes ...


8

Suggestion 1: Don't split the lines I don't see the need for splitting each line into tokens and extracting the numbers from each token. You can extract all the numbers from a line by using a istringstream with the entire line. Replace the lines boost::split(v, str, boost::is_any_of(" ,")); string xstr = v[0]; string ystr = v[1]; string zstr = v[2]; int ...


8

using namespace std; Never do that; certainly not in a header - that inflicts the harm onto every source file that includes the header. Prefer to include your own headers before Standard Library headers. This can help expose unsatisfied dependencies of your library's headers. Prefer <cmath> to <math.h> (etc.), as this puts the standard ...


7

As run() is several lines long, it should be defined outside the class. Anything defined inside is automatically inlined. Depending on the size of data when passing by value, it may be best to pass it to run() by const&, modify a local copy, and return that. RVO should still kick in. Is there a significance to the name foo here? If not, it should be ...


7

Optimize for maintainability first Is the complexity of your function really needed? The functionality can be implemented in one line using only standard-library algorithms. bool is_valid(const std::string& str) { return (std::count(std::begin(str), std::end(str), '.') <= 1); } Let's see how this will compare against your version. I hope we can ...


7

Here are some more things that bother me a little (adding to what yuri already wrote in their answer): Design Ugh, globals Globals are bad. They introduce hidden state that the execution of your functions depends on, are difficult to track, are prone to the static initialization fiasco and so on and so forth. Instead of making fbchart a global that is ...


7

class XYZCoordinate{ public: int x; int y; float z; XYZCoordinate(int X, int Y, float Z){ x = X; y = Y; z = Z; } }; Integrate a formatting utility, like clang-format or astyle, into your tool-chain. Your indentations and spacing are inconsistent. Do you plan on working with x and y as floats? Avoid transformations until you ...


6

I'm a little unsure about this line: default: assert(false); // Shouldn't be reached. Prevents compiler warnings. I assume that your compiler was complaining because there was no default case, which can happen in some situations. However, it feels odd to put an assert there, which is mostly used for debugging. You do say that that shouldn't be reached, ...


6

There's a few design issues with your code I think. Copying Copying your LinkedList performs a shallow copy. I think that would be counter-intuitive to anybody using your data structure. Typically in C++, copies are deep copies. On the one hand, using shared_ptr<Node> means you don't have to write any of the special member functions, so well done ...


6

3. The error handlers The problem with the throw expression, as the compiler kindly reminded you, is that they're void-expressions. Even if it compiled, it would not do what you want: it'd throw during the grammar constructor... The repeating story here is that semantic actions (and error handlers in this case) require Phoenix actors (a.k.a. lazy or ...


6

Missing Headers Before going any further, the current code is missing a few headers that it needs, namely: <functional> and <memory>. Navigating to Location doesn't work Next, I've ran the code against a site that redirects me to a different location and the second call resulted in an error: error_core.message(): A connect request was made on ...


6

The biggest problem I can see is that you are not using idiomatic Makefile names, which will confuse readers and frustrate users who want to change things via the command line. Typically CFLAGS is compiler flags passed to the C compiler, CPPFLAGS the preprocessor and CXXFLAGS the C++ compiler. $(CPP), $(I) and $(L) are are also just plain confusing and hard ...


5

Herb suggested another way for associating mutexes with data in c++11 more recently than the linked article in this video. The whole video is worth watching in my opinion, but he explains a wrapper pattern around minute 38 and shows how it applies to associating a mutex in minute 40. His implementation looks like this: template< class T > class ...


5

Per the comments, this isn't really a "pool" because things don't go back into it when you're done using them. It's just an "incremental allocator" that does allocation in "chunks". A better name for it might be shared_ptr_chunk_allocator (and chunk_size instead of pool_size). Given that pool_size is used only on the codepath that also allocates memory (so ...


5

Here are some small things you can improve: constexpr inline isn't necessary: constexpr already implies inline. In the following function: constexpr friend inline Arithmetic operator+(Arithmetic a, const Arithmetic& b) noexcept { return a += b; } The friend keyword isn't useful: the only function you ...


5

The use of doSomeSpecific inside of handleTimer is remarkably similar to the Template Pattern. If there is no base implementation of doSomeSpecific you might consider making it private (still virtual so it can be overridden). Unless of course you really want further derived classes to be able to call the doSomeSpecific implementations of their parent ...


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