New answers tagged

0

I would expect Slurp.cc to include Slurp.h - in fact, I can't see how you could compile it without the definitions there. I recommend making it the first include, to detect whether any dependencies are introduced. using namespace std; Bringing all of the std identifiers into the global namespace is usually a bad idea - just write std::size_t and other ...


2

Don't write using namespace std;. You certainly don't need to write it twice!! // your code goes here along with the duplicated using declaration makes me think that you did not really proof-read your own code first. Pay attention to what you're writing! It will go a long way to finding improvements and silly mistakes. #include <bits/stdc++.h> is ...


6

Here are some things that may help you improve your program. Use all required #includes Part of the interface is the #include files that are required, so those should be included here. Make your code hard to abuse Right now we're playing fast and loose with the assumption that memcpy will do the right thing, no matter what type is used for the template. ...


0

regarding std::list<int>::iterator i; for (i = adj[v].begin(); i != adj[v].end(); ++i) if (!visited[*i]) DFSUtil(*i); You have the definition of i outside the for statement and not initialized; you spell out the complex and specific type rather than using auto, you repeatedly index adj[v] and you call .end() each time through ...


2

Adding to what Rish wrote, don't use std::endl. Just use \n in the string to indicate a newline.


6

Your code produces no memory leaks since you're not manually allocating and deleting memory. Include all headers Always include all the headers. In this case, you need <iostream>, string, and cstring. Pass by const reference You should pass your objects by const reference. For example, const std::string& buf and const PacketType& packet. You ...


1

Starting from Quuxplusone's answer, let's get rid of some more operations. for(int a = 1; a <= n; ++a) { for(int b = a + 1; b <= n; ++b) { if( ((m%a)%b) == ((m%b)%a) ) count++; } } One thing to notice is that the quantity m%a never changes during the inner loop. So, we can move it to the outer loop. for(int a = 1; a <...


2

I'm sure the puzzle intends you to use more math and less brute force. But here's a 2x speedup right away: Any loop of the form for (int a=1; a <= n; ++a) { for (int b = 1; b <= n; ++b) { if (a < b) { do something } } } can obviously be replaced with a loop of the form for (int a=1; a <= n; ++a) { for (...


1

This is a good project, one that could be very useful in practice. I’ve had two or three C++ blogs over the years, and every time I spin up a new one, I include an updated version of a very old post I wrote about reading an entire file into a string. It’s not an easy problem to solve efficiently, for a number of complicated reasons. Eschewing the standard ...


0

I have an efficient function that splits a string based on a single character delimiter, and it does not need to allocate memory and copy the substrings, at all! First, it returns a collection of string_view so it just points to the locations within the original string. Second, it returns them as a std::array rather than a vector so it doesn't need to ...


1

Just a quick note: the begin and end iterator might be different types, as a recent improvement in STL provides for end to be a special "sentinel" type that's different from the normal iterator. If you're using C++20, you can use Concepts to declare the template arguments, to ensure that they are iterators (and sentinels, respectively) to the ...


2

You have received excellent feedback from others. I can only add to that in a show-and-tell fashion. What my version does is address all the issues mentioned by others: make the directed/undirected choice explicit in your graph make the algorithm iterative so you don't have the stack limit make your containers contiguous to reduce allocations and increase ...


4

There are a few things I would improve: Clearly separate the three distinct phases: Skip until the first index in the set Copy/delete until we reached the last index Copy the remainder until the end of the container is reached. Have an outer loop that loops over the entries of set. Use *foo++ syntax. Use auto return type deduction. Name things after their ...


1

Use non-member functions where possible: The cast functions are all static, and there is no member data in SafeNumericCast and SafeRuntimeNumericCast, so these the classes don't need to exist. We could use namespaces, or plain non-member functions instead. Unnecessary inheritance: Inheritance seems unnecessary and confusing here. SafeRuntimeNumericCast ...


1

template <bool isConst> class ImageIterator { public: ImageIterator(cv::Mat data, size_t initial_index) : data(data), index(initial_index) {} ... private: cv::Mat data; size_t index; ... }; It looks like cv::Mat is a reference-counted handle type, so copying a cv::Mat doesn't copy the actual data. However, this also implies that ...


1

~ThreadPool() = default; We need to call stop() in the destructor! template<typename Func, typename... Args> decltype(auto) add_task(Func&& func, Args... args) { using ReturnType = decltype(func(args...)); For perfect forwarding we need to use && for args: template<typename Func, typename... Args> decltype(auto) add_task(...


1

Avoid using macros Try to avoid macros where possible; usually a better solution is available. For constants, prefer using constexpr: static constexpr float PI = 3.14159...f; Instead of MIN and MAX, just use std::min() and std::max(). Or if you can use C++17, use std::clamp(). Constructors and member initialization If you need to explicitly add a default ...


0

I mean no offense but this is one of the worst implementations and least efficient implementations of split. Think what this operation does input = input.substr(pos + 1);. It copies all data past pos and store it in another instance of a string. Basically, if input had N strings each of size K then this algo would take O(N^2 K) operations which is too much. ...


3

What are the expensive operations you do? Allocating memory for the copy of a std::string passed into splitString(). This can be fixed by accepting a std::string_view instead, or at least a constant reference instead of a copy. See How exactly is std::string_view faster than const std::string&? and What is string_view? for details. Allocating memory ...


0

If you don't want to modify the original string, you can use a stringstream: #include <vector> #include <sstream> std::vector<std::string> splitString(const std::string input, const char& delimiter) { std::vector<std::string> elements; std::stringstream stream(input); std::string element; while (getline(stream, ...


0

As you got TLA, you (probably) need a better algorithm: Map from the strings to their periods. Remove duplicates. Map from string-periods to transformation-periods modulo 109 + 7. Determine LCM over them modulo 109 + 7. Regarding implementation efficiency: You allocate memory everywhere, for std::string and std::vector. Both are expensive, and also ...


2

Just a few quick observations: /* sqlite constructor */ Duh. Keep comments useful and meaningful. You don't need to explain that the ctor and dtor are those things. (const std::string& table_name, Suggest you use std::string_view (by value) for parameters instead. This is more optimal if you pass a lexical string literal, as it doesn't have to copy ...


-1

#define MOD 1000000007 Don't use #define. This should be constexpr int MOD = 1000000007; (and this is assuming that ints are big enough to hold that value!) int findSmallestString(string A) Why are you passing A by value, duplicating the string? Write const string& as a matter of course; but use string_view for parameters when that makes sense. int ...


2

It looks like others already covered everything I noticed, except for the way you position the * in a declaration. From the beginning, C++ has used the style of putting the pointer (or ref) mark with the type not with the variable. Form* myform; And looking at the main function where this example came from, notice that it is not being initialized, though it ...


2

The signum function returns -1, 0, 1 if the argument is negative, zero, or positive. int original_diff = signum(to-from); When signum(to-current) is diferent, we know the relationship changed, regardless of which direction you were going. if (original_diff == 0) return; //do nothing for (int current= from; signum(to-current) != original_diff; current += ...


6

Design review I’m not going to bother with a code review, because I see @G-Sliepen has already done an excellent job of that. Instead, I’m going to focus on a high-level design review. The name of the pattern you are using is the Factory pattern. The Factory pattern uses a single “make” function that takes an argument, and returns a different object type ...


2

The only difference is the </> which swaps on multiplication by a negative factor: x < y <=> -x > -y. An ugly use would be: for (int i = from; signum(step)*i < signum(step)*to; i += step) { for (int i = from; signum(i - to) == signum(step); i += step) { for (int i = from; (i - to)*signum(step) < 0; i += step) { for (...


9

Use all the power of the standard library In your post on StackOverflow, you mentioned that you couldn't use std::map because you were not allowed to use it. Outside of the classroom, you should not limit yourself this way, and instead use the appropriate functions from the standard library that help you write concise and efficient code. The function func() ...


3

Form *func(std::string name) { std::vector<s_makeForms> allForms = { {"triangle", Triangle::createForm}, {"rectangle", Rectangle::createForm}, {"square", Square::createForm} }; for (const auto& form : allForms) { if (name == form.formName) return (form.makeForm()); } return ...


4

I think the easiest way to solve this is to first calculate how many steps it takes to go from from to to, and then just build an array of that many elements. That also has the advantage that you can reserve the right amount of elements in the vector up front. You also have to make sure that step has the correct sign. if (from <= to != step > 0) ...


1

Constants const int MIN_CAPACITY = 16; const int GROWTH_FACTOR = 2; const int SHRINK_FACTOR = 4; These are polluting the global scope. I would put them into the class as named constants: template <class T> class Vector { enum { MIN_CAPACITY = 16, GROWTH_FACTOR = 2, SHRINK_FACTOR = 4, }; // ... operator= and friends. ...


3

Sort the includes to avoid loosing track. Instead of manually providing BASE and DIGITS10BASE, use std::numeric_limits<> to derive them as needed. Generally, passing a scalar type (pointer type, any fundamental type) by constant reference is premature pessimization. It opens the can of aliasing, adds an indirection for access, and rarely saves space. ...


3

Your code labors under the assumption that exceptions cannot happen. Unfortunately, they do. You default initialize all your elements on allocation. Not only is that potentially a big waste of time, it also might be impossible (compile-error) or flat-out wrong (silently wrong behavior). To fix that, separate allocation from constructing the elements. By all ...


1

const int MIN_CAPACITY = 16; const int GROWTH_FACTOR = 2; const int SHRINK_FACTOR = 4; these should be part of the class, not global variables that affect everything and pollute the namespace when your header is included. And use constexpr now. _data(new T[MIN_CAPACITY] No naked new! Use a unique_ptr<T[]> instead of a bare pointer for _data. ...


2

if(a.negative!=b.negative){ return false; } return std::equal(a.container.begin(),a.container.end(),b.container.begin(),b.container.end()); } For the last line, doesn't vector's operator== do what you need? So you end up just checking if all the data members are equal. So use the C++20 capability to autogenerate it. But you mentioned using ...


1

Others have given you good advice on your style. I am far to rusty at C++ to comment either way. I suggest the approach below to actually solve the problem presented, and then apply the advice from others to make it good C++. #include <iostream> #include <cstdlib> #include <cmath> void falling_ball(double height) { const double g = 9....


2

Remove redundant comments You added a lot of comments to the code, but many of them are not very useful. Comments should be used to explain what the code does if this is not clear from reading the code itself. But for example: std::queue<Task> _toDo; I can see just from this line of code that this is a queue of tasks to be done, so the comment you ...


1

int i = 1; while (i < arr.size()) { // body of loop ++i; } That is the quintessential for loop. Write it as: const auto ASize = arr.size(); for (size_t i= 1; i < ASize; ++i) { // body of loop } I also saved the array size since it doesn't change... but your variable arr is not const and the compiler can't really deduce ...


3

In addition to what's been posted previously, I like to stress that such problems should separate input, "the real problem", and output into separate functions. In your case, the print-as-you-go is more trouble than it's worth to isolate, but you can easily separate the user prompt and have a function accept a parameter. This makes it easy to call ...


3

@user673679's comments about style, var names, and the weirdness of having a static int inside a special function are all good so I won't repeat most of that. You over-estimate the final speed by up to 0.9999... seconds of extra falling time past ground level, i.e. worst case error of almost 9.8 m/s if the previous timestep had the object above the ground by ...


3

using namespace std; There are lot of good answers why you shouldn't use it. See here Use std::pair<int, int> Since your result struct is just acting as a container for two ints, consider using std::pair<int, int> Don't hardcode values Why do you want hardcode 16 as the array length? You can just pass it to the function as a parameter, or use ...


2

Inside loop, you're checking if some code should be run before startup. You could simply move the corresponding code to setup, and be sure it's only run once: #include <AccelStepper.h> AccelStepper stepper1(AccelStepper::DRIVER, 9, 8); AccelStepper stepper2(AccelStepper::DRIVER, 4, 3); int side = 0; int max_speed = 6000; int max_acceleration = 12000; ...


2

Your insertionSort() is OK, but I would write: template<std::forward_iterator Iter> void insertion_sort(Iter first, Iter last) { for (Iter it = first; it != last; ++it) std::rotate(std::upper_bound(first, it, *it), it, std::next(it)); } insertion_sort(v.begin(), v.end()); display() copys your vector, so make the parameter as const (lvalue)...


7

Questions (+ design review) Could anyone do some little code review please? The biggest issue I see with your current design is that you are using the “god object” anti-pattern. Your Sorter class: Loads the data from a file. Implements over a half-dozen sort algorithms. Tests over a half-dozen sort algorithms. Writes sorted data to a file (over a half-dozen ...


2

Here are some things that may help you improve your code. Make sure you have all required #includes The code in game.h refers to std::ostream and std::istream but doesn't #include <iostream> where those are defined. Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation. ...


4

Since everybody here has already provided great suggestions. I am going to try and answer only the asked questions. Remove duplicate code: have internal function bool defineCommandInternal(const char *name, const __FlashStringHelper *doc_fstring) { const char *doc = (const char*) doc_fstring; if (commands_count < MAX_COMMANDS) { commands[...


2

Define simple member functions in the header files If you define member functions inside the class definition, they can be inlined by the compiler. For simple functions, like CountDown::is_finished(), this will generate much more efficient code, even on a Z80. Unnecessary casts to std::string There are several unnecessary casts to std::string in your code, ...


11

using namespace std; is a bad habit to get into, as it can lead to name-collisions and other issues. It's best to explicitly qualify the names we need where necessary, e.g. std::cout. A few more line-breaks would be useful in the code. Think of code with a specific purpose as a paragraph when writing text. For example, this could be considered one "...


1

Dealing with the lack of STL You wrote: @JDługosz's answer contains many good tips for generic C++ projects. Those suggestions don't seem to apply to Arduino projects, though, because the std:: objects are not available. I'd be glad to get suggestions specific to the Arduino platform, so that the code can be compiled for ESP32/ESP8266, at best without ...


3

From my experience working with Arduino or the AVR architecture in general is an exercise in trying to make sure everything you need will actually fit in memory, both RAM and flash. The scary thing is you don't really know how much space your stack will require so there's a bit of a guessing game if your stack is going to overwrite your heap or static data ...


Top 50 recent answers are included