New answers tagged

2

Overview So I created a thread server that creates threads and associates them with a handle so that you can keep specific threads for specific tasks e.g. run all graphics rendering on thread 0, run physics stepping on thread 1 and use thread 2 for downloading a file of the internet etc. I don't think that is a good idea in general. Though it is an easy ...


9

Overall Observations If I were a teacher I would give you an A+ for effort and about a B- for implementation. From a design point of view try to separate the logic of the game as much as possible from the display of the game. Real gaming companies will do this to be able to distribute the same game for multiple platforms. This would also allow the use of the ...


0

The task is to find if there is a path between the start and end points on a map for a given set of problems. That's the key to doing this efficiently: all you need to do is figure out if a path exists, you don't need to find out what that path is. An easy way to do this is to color the map: load the map into memory, then use the flood fill algorithm of ...


2

Remove class IEventHandler Unless you plan to have multiple classes that inherit IEventHandler, there is no point in declaring an IEventHandler and having EventHandler inherit from it. Avoid duplicating functionality In the constructor that takes a Message *, you duplicate code from OnMessage() and startEventHandlerThread(). It is always best to avoid code ...


2

Overview I started the code review but stopped half way through. This is all wrong. A reactor should hold a thread pool (or a single thread to do the work). When an event is triggered a thread is released from the pool and simply calls the OnMessage() method of all appropriately registered handlers. You are confusing the event handler pattern and the reactor ...


4

Most obvious optimizations - check if start point and end point are the same. If they are differs then neither of citizens can move. Second - flatten your map. You can have just one contiguous vector size of r*c elements and points can be "flattened" to index as point.x + width * point.y. So flattening map allows you to flatten your start points ...


3

Great answer by L. F.. With regards to optimization it can even be done in O(n) either by using a hash (assuming there aren't too many hash collisions): std::vector<size_t> findNumbers( std::vector<int> const & nums, int const target ) { // pack numbers and indices into hash std::unordered_map<int,...


6

Question Does the last line slow my program down? This depends on what you intend to return when there are no pairs that sum to target. If you leave the last line out, your program would invoke undefined behavior when control flow reaches the end of the function body, and the compiler will rightfully warn you about that. Integer overflow There is an edge ...


1

Fix compiler warnings The compiler will warn about some unused parameters and of comparisons between integers of different signedness. For the latter, the solution is simple; instead of: for (auto i = 0; i < some.size(); i++) { Write the type of k explicitly: for (size_t i = 0; i < some.size(); i++) { The unused parameters of build_tree() should be ...


2

Overall Observations The function main() is too complex and should be broken up into multiple functions, if you do this you might notice that you are recreating your segtree struct for every query when it only needs to be created once and then each query should be applied to that structure. Recursion is expensive in terms of resources it might be better to ...


2

Your implementations of convertTypeToString and dimensionAsString are a little inefficient and unnatural. First, there is no need to use a variable inside the function which you assign to and finally return. Second, you should not represent an unknown or nonacceptable value as a string. Think about it from a user perspective - it's not nice (nor robust to ...


2

Two comments. First you can move the assignment down a bit to save some work. while(true) { //use a constant m_read each loop uint32_t read = m_read; //empty if(read==m_write) return false; if(__sync_bool_compare_and_swap(&m_read,read,read+1)) { t = m_array[read&MASK]; return true; } } Second, if you declare m_read as ...


5

Keep it simple Use the KISS principle, and don't make things more complicated than necessary. Using more language features should not be a goal in itself. Also, don't add more things in a class than necessary. Why do you keep track of dimension? Nothing inside the class makes use of it. The dimension is a property of PointType and VectorType, so it's already ...


2

The iterator is not behaving correctly There are various reasons std::copy() doesn't work on stack_stack, and it all has to do with the iterator. First, you are missing difference_type. Since your iterators do not support taking the difference, set it to void: using difference_type = void; Second, your comparison operators are wrong. They should take a ...


2

General Observations As a utility this could be made more usable and flexible if it took command line arguments, such as the file to process and the number of lines to print rather than having them hard coded in the program. A well written utility would also handle the part done by hand in an editor. There might not be any reason to process a JSON file. The ...


7

About the copy constructor and resize() In the copy constructor (and the other constructors as well), you allocate memory using new T[...], but in resize() you allocate memory with new char[sizeof(T) * ...] and then use placement new to copy the old elements. The former is safe, but potentially calls more constructors than expected, the latter has the ...


0

It takes more processing due to mAnimation inside paintEvent() method.. So remove Create animation on from paintEvent(). generate animation inside mouseReleaseEvent() method only.. mAnimation->setDuration(100); if(mAnimation->state() != mAnimation->Running) { mAnimation->setPropertyName("mOffset"); } mAnimation->...


0

Now without a map: #include <cassert> #include <functional> namespace nlm = nlohmann; class properties { using serializor_t = std::function<nlm::json()>; using deserializor_t = std::function<void(nlm::json)>; struct property_info { std::string_view k; serializor_t serializor; deserializor_t deserializor; }; ...


3

Use your own typedefs I see this code: typedef std::list<Edge>::iterator iterator; ... std::list<Edge>::iterator begin() {return edges.begin();} You should use your own typedefs, so the second line becomes: iterator begin() {return edges.begin();} Apart for less typing for yourself, it also avoids possible mistakes where you return a different ...


3

Avoid creating namespace aliases in header files I assume that at least the declaration of class properties would be put in a header file. In that case, consider that users of that header file might not expect namespace nlm to be declared, so I recommend just writing out nlohmann fully. Spelling A minor issue: it is serializer, not serializor. Function names ...


-1

I really liked your design, but for a reason that I'm not sure to understand it actually yields the same value for any type (your static_assert breaks). Perhaps it's due to my compiler, Visual Studio 2019, breaking the technique because of its identical COMDAT folding linker optimization, like in this answer from another similar question. Basically the ...


0

std::endl flushes the output buffer so it slows the printing of characters, you can use '\n' which is a char ("\n" is a const char*), take it into account. It is good to mention that using namespace /*the namespace*/; in a source file makes the files which will include it to also use /*the namespace*/, in general it is bad. You could do instead: ...


1

It's an old review, but I tried the code out, so, some pointers: keylogger.h is missing #include <ostream> - your code is not complete without it. Instantiating the keylogger with a pointer to an ostream without checking for nullptr is unsafe. Take it by reference instead. You can still store a pointer though. Every keylogger instance will overwrite ...


2

1. Names/Magic numbers What is the magic number 55 in your base converter function? I suggest giving it a name. 2. Bounds / Magic numbers / validation Also, what are the upper and lower bounds on which bases are allowed? Only 2-9 since you are comparing with 9? That should also be specified and checked when reading the input. 3. Code formatting / style Your ...


1

CORRECT Endian detection: With the great help of Fabio A, I was able to check several codes that claim to work. However some of them do not work and some works only in gcc. Seems the only working way to detect endian-nes on compile time on both gcc and clang is as follows: constexpr static auto check__(){ #if defined(__BYTE_ORDER__) && ...


5

Consider using std::variant So what you want is something that can either hold a single value or a std::vector of values, with the idea that you don't want to pay for the memory and/or CPU overhead of a std::vector for a uniform field. Consider using std::variant: template<typename T> class Field { std::variant<T, std::vector<T>> v_; ...


1

The isBE check is wrong. What you're actually doing is a cast from uint32_t to uint8_t, which doesn't give you any info about whether the system uses big or little endian: in either case the result that ends up in isBE::value is 0. The correct way of doing it would be to do a reinterpret_cast, alas that's prohibited with constexpr. Unfortunately, a constexpr ...


4

For something easy to read and maintain I'd use set_difference and set_intersection which would work well on sorted ranges with no duplicates: std::set_intersection(a.begin(), a.end(), b.begin(), b.end(), std::back_inserter(both)); only_a.reserve(a.size() - both.size()); std::set_difference(a.begin(), a.end(), b.begin(), b.end(), std::back_inserter(only_a));...


1

Undefined behavior There's UB in case when empty vector passed to the binary_search. unsigned int mid; it is uninitialised and will pop whatever compiler think he want in release mode. To solve this issue it is require initialise value with some error value (usually it is ~0 but you can choose your own), or use std::optional (c++17) and handle error case in ...


4

As of C++20, you can use std::is_constant_evaluated(), which is more portable and considerably easier to read. I realize you have only tagged C++17, but I just wanted to point out that there is now a cleaner solution to this problem. #include <type_traits> #include <cmath> namespace example { namespace impl { template<...


2

Consider making Systeminfo a POD I would separate getting information from the SMBIOS from how to store the result. Make Systeminfo a plain old struct: struct Systeminfo { std::string family; std::string manufacturer; ... }; And move the original constructor into a plain function: Systeminfo get_systeminfo() { Systeminfo info; ... ...


7

First: This seems like an application of "The OO Antipattern". I don't see why you need class Palindrome at all; and if you must keep it, you certainly shouldn't store the entire dataset — just process it once in the constructor and keep the palindromes! Similarly, Palindrome<T>::Print() seems like it ought to be generalized to "print ...


2

I. Construction is split in several phases. Or, in other words, a freshly constructed object is not in a finalized, ready-to-use state and needs one more explicit initialization call. This is a very strong antipattern, which could obviously lead to errors. A more decent solution would be to filter the dataset right inside the constructor, and not keep two ...


2

Martin R already made get_sum_of_factors a lot faster by going only up to sqrt(n). You can do even better by using prime factorization as shown below. This also at most goes up to sqrt(n), but reduces n and thus sqrt(n) in the process. Here are times for computing the sums of factors for num from 0 to 1,000,000 with the sqrt-method and with my prime-method (...


27

About using typedefs First, don't create aliases for standard types. Just write std::string instead of str. For someone reading your code, or perhaps you yourself reading your own code half a year later, whenever one reads str one wonders "is this a std::string or some other kind of string?" Furthermore, it is not good practice to introduce very ...


8

In my opinion, the best way to improve this program is by using bitboards. Instead of using a table in two dimensions to represent the chess board, you use 12 numbers of 64 bits, each number representing a type of piece and each bit saying whether there's a piece or not on a square. You can then use bitwise operators to modify the chessboard. This method is ...


1

After implementing both of the suggestions given by Miguel Avila and Martin R, the time taken to calculate was significantly faster. Original code #include<iostream> int GetSumOfFactors(int num){ int sum = 0; for(int i = 1;i < num/2+1;i++){ if(num % i==0){ sum+=i; } } return sum; } int main(){ int ...


0

This can be rewritten using regular expression \s*(\d)\D* (zero or more whitespace characters, a single digit, zero or more non digit characters. If I understand correctly, heading and trailing spaces are ok, 2ff is ok but ff2 is not ok and 23 is not ok either. int input_number() { static const std::regex valid_number("\\s*(\\d)\\D*"); string ...


1

You have a rather long if-else ladder. I think this can be simpler written as: switch (n) { case 0: std::cerr << "Notified" << std::endl; return true; case ETIMEDOUT: std::cerr << "Timed out" << std::endl; return false; case EINVAL: std::cerr << "Invalid value ...


3

I'll add some remarks (adding to what Miguel Avila already said) and then focus on the performance aspect. Use consistent naming: you mix camel case (FactorsSum) and snake case (sum_of_factors). Use consistent spacing (after keywords like if and around operators). Declare variables and the narrowest possible scope. As an example, sum_of_factors is only ...


0

The checkBit function is very verbose. It can be much shorter and maybe even faster without losing clarity: bool checkBit(unsigned char cell, unsigned char mask) { return cell & mask; } In general you use int for boolean values but there is a new bool type in C++ now that I would recommend.


0

After all the suggestions I turned my old code into the following: #include<iostream> #include<vector> void printvec(std::vector<int> vec){ for(int i:vec){ std::cout << i << ' '; } std::cout << '\n'; } std::vector<int> sortvec(std::vector<int> &vec){ int temp = 0; for(long ...


2

I would suggest to use the newer C++ stuff at least where it very obviously simplifies the code. The int tmp = arr[i]; arr[i] = arr[i-1]; arr[i-1] = tmp; can be written in one line instead of three: std::swap(arr[i], arr[i-1]); std::array is just the same as the plain old array but knows the own size: std::...


3

Below is a non-comprehensive review of your code. Choosing a maze generation algorithm There are many algorithms for generating mazes, each with their own pros and cons. If you really need to create huge mazes as fast as possible, your backtracking algorithm might not be the best. However, each algorithm typically has its own bias for generating particular ...


2

Make sure you add #include for everything you use directly Your code happens to compile without errors because some other header file #includes the necessary header files for you to be able to use functions like std::acos() (from <cmath>), std::abs() (from <cstdlib>), std::min() (from <algorithm>), std::unique_ptr (from <memory>), and ...


4

There are few aspects which I will touch. The function GetSumOfFactors could be renamed as FactorsSum, it is done to simplify the reading. You are declaring a for loop for(;;) (equivalent to while (true)) but that is quite bad, one generally includes the ending statement in the for, as the variables updates and, if used only there, for loop scoped variables....


9

First thing first, make it into a function, such that your main will be int main(){ const int siz = 6; int arr[siz] = {4,6,3,1,3,8}; std::cout << "Before sort\n"; printarr(arr,siz); sort(arr, siz); std::cout << "After sort\n"; printarr(arr,siz); } Since you've tagged it c++, do not use c-style ...


5

That's not bubble sort. More like an overly eager selection sort. Bubble sort swaps neighbors.


3

It's wrong. For example for input {5,9,3,7,2,6} you print 5 2 3 6 7 9. It's not bubble sort. More like an inefficient insertion sort. It's not O(n2) but only O(n3). For example for input int arr[100] = {99,98,97,...,2,1,0} you have 161,799 iterations of your loop (that's 100C3 + 99).


Top 50 recent answers are included