New answers tagged

1

The thing I would focus on to start with is reducing code duplication. If you look at your triplechecko/x functions, it's basically the same code over and over but with different variables. I would start by replacing all the p1...9 with a container. Since it's fixed size, let's use std::array. Then lets see what we can do to reduce some of the code ...


1

Avoid Using Namespace std If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because ...


1

#include <iomanip> #include <iostream> #include <vector> void printHeader() { // You don't need to call cout multiple times, because the compiler will concatenate strings automatically // (more of a preference thing, but I prefer doing it this way) std::cout << "***********************************\n" "Lab ...


0

Some general comments: In terms of readability and protection from errors, you could benefit a lot from something like struct point which capsulates a 2-dimensional points. That is, if x and y belong together logically, they should be inside the same structure. Read about const correctness. All parameters that are not modified should be marked as const, ...


2

Overall, I think the code looks good. Here are a few sugestions that may help you improve your code. Check for overflow As you know, terms of the Fibonacci sequence get very big very quickly. For this reason, it's easy to overflow the numerical range. I'd suggest addressing this by throwing an exception in iterator& operator++(): if (b_ < a_) { ...


3

You could use a range-based for loop and push_back( Pressure(i,i) ). #include <iostream> #include <vector> class Pressure { public: Pressure(int date, int time) : m_date(date), m_time(time) { } int GetDate() const { return m_date; } int GetTime() const { return m_time; } void AddTime(int val) { ...


0

Prefer constants to macros Preprocessor macros are tricky to use correctly - they have no namespace, no type and global scope. We can replace SIZE with a private constant member: #include <cstdint> class Stack { // private constexpr std::size_t array_length = 50; } I renamed it, because we might later want to creat size() and capacity() ...


2

Overall, this looks pretty good. If it were me, I'd use a more descriptive name for the top of the stack such as topOfStack or just top. tos doesn't really mean anything, so without a comment describing it, I would be confused seeing that in code. (And if you need to write a comment to explain a variable name, you've probably named it incorrectly.) I'm not ...


4

The cost of starting up a thread is generally considered expensive. For your example of only 10 items, you won't ever see it because the application quits before you notice it started. Generally for something like this, you'd want to start up a thread to process some large portion of the array rather than one thread per element of the array. At 10 elements, ...


1

I reduced the time of the algorithm by ~36% by reducing the algorithm and improving the Cache. Tested for N=500,000,000. First lets look at the properties of the Collatz sequence 1) If the number is even, then we will perform a right-shift until the value is odd. 2) If the number is odd, then it will take one iteration of n=3*n+1 to become even. Proof ...


1

First, thank you for providing the link to your GitHub repository, it allowed a more complete review. I've noticed a real tendency in the code to avoid creating classes and to use procedural programming rather than object oriented programming. Namespaces are used instead of creating classes. The use of classes and objects can be very powerful, for one thing ...


3

Don't do using namespace std; Qualify names properly with std:: where necessary. string getProdName() const { return prodName; } int getQuant() const { return quant; } Avoid abbreviations as they harm code clarity. Most IDE's provide auto-completion, so abbreviations don't save typing anyway. receipt(const string& prdNm, int qnt) : prodName(...


1

I have a number of suggestions which might help you improve your program. Rethink the name The name Logger makes me think of creating a log which is a record of something that can be reviewed later. However, that's not what this does. It emits color coded messages to the console instead, so perhaps Logger is not the best term, since there isn't any ...


2

You are more interested in the performance, and on style there are sufficient mentions. The Collatz sequence can be reduced: if (iCpy & 1) { iCpy *= 3; ++iCpy; ++ct; } iCpy >>= 1; ++ct; while (iCpy != 1) { if (iCpy & 1) { iCpy *= 3; ++iCpy; ++ct; } iCpy >>= 1; ++ct; } As a loop's intermediate ...


3

You lack several features: logging to files, not just CMD; to this end you'd better make an interface class for logger and pass only pointer to the interface. Otherwise you'll end up with too many if/else and hard-to-maintain code. option for printing the time of the message. Even the C++ style support isn't available in C++17 there is a C-style version you ...


2

The lower loop (which fills all power-of-two multiples of the outer index) is doing unnecessary work when i is even. If we find that vals[i] was already filled in by a previous iteration, then so was vals[2*i] and so on. I improved run-time by about 5% simply by adding at the start of the outer loop: if (vals[i]) { continue; } It's usual ...


2

In addition to the command line, there are at least two other methods to pass this information in: the .ini file was mentioned in another answer, environment variables are also an option. It's best not to force the user to type in too many options for the command line. Avoid Using Namespace std If you are coding professionally, you probably should get out ...


2

I tried boost::program_options but simply didn't like it. boost::program_options has the feature that certain options are additive or hidden - which is why their usage requires so much extra work (you have to specify each option you use and state how they are processed). If you are fine without these features, you can work with config files - *.ini format - ...


8

Euclidean distance is used to calcualte the cost from current node to goal The code implements Manhattan distance, so the comment is wrong, or perhaps the code is wrong, but in any case it doesn't match. You can use this page to review heuristics for grid worlds, I don't recommend Euclidean distance because it's either too optimistic (causing unnecessary ...


5

Just off the cuff, before I fully code review this, I would tell you: 1) There are not enough comments. You add comments like this: //string vals; cout<<"Enter max_step_number for the random planner: "; ...these are obviously string vals, but what are these? motion_planner mp; int x,y,gx,gy,r,c; int val; 2) You need to initialize your values. 3) ...


3

A bit late to the party, but I spotted a few mistakes in this otherwise interesting exercise and I thought I would highlight them. Trying to run the given code as-is results in crashes. Here's why. First and foremost, std::vector iterators get invalidated whenever an element is erased from the vectors, unless the erasure happens at the vector's very end. ...


3

Prefer returning a result instead of passing it via a reference The result of your function is a std::ifstream object, so make that explicit: std::ifstream OpenFile() { while(true) { // read filename std::ifstream file(fileName); if (file.is_open()) return file; else // handle error } } The advantage is that instead of ...


0

It looks bad on multiple levels. It doesn't look like a decent C++ but closer to C. Use std::vector, std::map, and their functionality instead of memset... also passing ll tree[] as input parameter of a function is ill advised. Usage of recursion, it screws your performance and it is probably the source of bugs. Also static allocation of large arrays a and ...


4

Sink arguments (objects we want to make a copy of and store internally) are generally passed by value, then moved into place, so the add function can take its parameter by value rather than const&. We could perhaps make fp public with a name like Function, and use it for the add function parameter. raise will make unnecessary copies of the arguments. We ...


1

Consider replacing the while loop with two for loops that increment lower and upper iterators till they have a suitable value - and only afterwards setting closest - it might be doing more operations but it is more cache friendly. Or simply create a new map2 with only valid values (simply filtrate unwanted). Though, different situation have different ...


2

Instead of #define use enum or constexpr for the constant numbers. I don't think that the number of time data or number of points should be absolute constants - rather they should be determined during runtime. Else you have to recompile your code each time you receive new data. Better declare some sort of output path instead of relying on ../, also you need ...


3

There are two general approaches. The first is to use image, and modify as you see fit. Pros: Simple to use. Can be easily applied to all shapes. Reliable. There might already be a free suitable implementation you can use. Cons: Fixed limited accuracy. You might need to implement a GPU or SIMD based code to achieve required performance. Requires too ...


2

Here are some suggestions: Don't use global variables. Instead of using a single big function, break it down to logical parts. const int N = 1000000; should be constexpr instead of const. Instead of = {0}, use {}. The latter makes it clear that you are value initializing all elements. i should be declared in the loop. Post-increment operators i++ should be ...


2

Welcome to Code Review! lexer.hpp It is common practice to put standard headers before custom headers and sort them in alphabetical order: #include <unordered_set> #include <vector> #include "reader.hpp" Usually, class names are capitalized. In general, try to avoid abbreviations. The token kinds are more readable in full name: enum class ...


2

Generally looks okay (though I would prefer shorter line lengths, and the indentation needs cleaning up). It seems that we've focused on using std::back_insert_iterator as the output iterator; this leads to a couple of problems that are visible when we switch to a different iterator, such as a collection iterator or raw pointer: The caller doesn't know ...


2

Regarding your typeid, you have to include the header <typeinfo> before being allowed to use it. In your any_big and any_small: Why does the function static void* Type<T>() return a void*? You could return a const std::type_info& instead Your any_big_obj and any_small_obj are variables defined in the header-file. If you include your any.h in ...


13

Unused headers As far as I can tell, nothing from the following headers is actively being used, so they shouldn't be included. <ctime> <cctype> <cstdlib> using namespace std While it's probably not quite as bad using it inside a .cpp file instead of a header, it's generally seen as a code smell. Variable usage There are some variables ...


3

There isn't a lot I see that can be improved; nevertheless: Initialization vs. execution Your clientController::init is misnamed. It does both initialization as well as running the main application loop. It'd probably better if you separated those two functions. This code: //**** This is the part responsible for connecting the signals and slots for the ...


1

Not too much to review as the code uses evhttp to handle HTTP and provide a basic, static reply for each request. Why is host constexpr, but not the port? All those using statements, combined with traits and decltype one after the other makes the code very hard to read, almost obscuring the fact that evhttp is being initialized at that point. I would try ...


4

This destructor is pointless: ~GraphFactory() { mGraphDataMap.clear(); mCurrentPath.clear(); mTotalPath.clear(); } Those three members are going out of scope anyway, so clear() is redundant. Just omit the destructor entirely. Here's a useless statement: if (mPath.empty()) { pathToParent; } Was that meant to be a return? Use ...


5

Coding for performance doesn't mean that you should write unmaintainable code. Remember that developer time is valuable, too: any time you spend tracking down avoidable bugs is time you could have spent tweaking program performance. Some particular things that can be improved without impacting performance: Include the headers for standard identifiers we ...


3

I'm surprised that, according to your code, C++ doesn't have a strip_prefix or without_beginning or skip_start function to skip over the initial "0x". If there were one, you should definitely use that instead of implementing your own. Do a little search, I'm sure such a function exists. Even though this is C++, there is no need to write a class for a simple ...


4

Exploiting sparseness is a nice trick, though not as good as Hashlife (it's in a class of its own). Hashlife aside, there are simple approaches that work well on actual computers, not by being algorithmically clever but by being computer-friendly. Compared to a a completely plain approach, yours is definitely better. But it does not really tap into the ...


5

The code may be over optimized already and adding additional optimizations will only make it harder to read and maintain the code, but that said: Missing Optimizations When printing prefer '\n' over std::endl because std::endl calls a system function to flush the output, '\n' just inserts a new line. Put the check to print outside the function ...


0

Sorry that I'm four years late, but I'm very interested in this topic. I hope you are still interested in it as well. WHEEL FACTORIZATION That would greatly speed up your code. The canonical examples you reference use wheel factorization, so I'm quite sure you are aware of it. Perhaps you feel it's too large or complex to add here and might confuse ...


2

std::strings can be passed by const& (or as a std::string_view) rather than by value if we don't need to make a copy, e.g.: Shader(std::string const& vertex_shader_path, std::string const& fragment_shader_path) std::string get_contents(std::string const& file_path); void Shader::print_error(std::vector<char> const& error_message, ...


2

Note that // Operator that combines two enumeration values into a flag_set. template <typename T> std::enable_if_t<std::is_enum<T>::value, flag_set<T>> operator|(const T& lhs, const T& rhs) adds operator| to all enums, including those that don't have the expected special sentinel. This breaks e.g. Qt's Q_DECLARE_METATYPE() ...


6

You've got some issues with your perfect forwarding and move semantics. You wrote: template<class T, typename VT = std::decay_t<T>, typename = std::enable_if_t<!std::is_same_v<VT, any> && std::is_copy_constructible_v<VT>>> any& operator=(T&& rhs) { any tmp(rhs);...


3

Let's go through the code and see what can be improved. #pragma once This shouldn't be in a non-header. #include <list> #include <map> #include <string> #include <variant> #include <vector> #include <fstream> #include <iostream> #include <sstream> Sort the include directives in alphabetical order. ...


0

We can use the standard library for equality comparison: #include <algorithm> constexpr bool operator==(const SmallVector &other) const noexcept { return std::equal(begin(), end(), other.begin(), other.end()); }


1

Here is my unpopular opinion: This class has no reason to exist and you should use std::array instead which does what you need. If you need it to be compatible with std:: vector for template meta programming, you can just add a thin wrapper on top or use SFINAE to call reserve only if it exists.


1

Here's my suggestions: Design Calling the constructors and destructors at the correct time with constexpr is indeed nontrivial, but disrespect object lifetime != POD. Your implementation is conceptually fine with some non-POD types. Maybe "trivial" is what you are looking for. You are missing a lot of functionality. Users may want to use them, so you ...


3

The two conversion constructors are not declared in the class, nor are they declared inline. If this code is in a header that is included by multiple source files, you'll have an ODR violation, which typically results in a linker error for multiple definitions of a symbol. Those two constructors should be declared with the inline keyword. inline Radian::...


8

Design While it might seem that the current design is working fine, I think there are two distinct issues lurking beneath the surface. Issue #1: operator double From what I can tell, the intention behind these unit classes seems to be the prevention of unit mismatches. Providing an access function to the contained value is not a bad idea, but using the ...


2

Thanks, @Xaqq, this is really nice! I published the code in GitHub with amendments from @Barry and @lightness-races-in-orbit and minor modifications, usage is as follows: #include "flag_set.hpp" enum class Options : uint8_t { FULLSCREEN, INVERT_MOUSE, FLASH, RED_BACKGROUND, RED_FOREGROUND, ‿ }; int main() { flag_set<Options&...


Top 50 recent answers are included