New answers tagged

0

The equation given by Binet: fib[n] = (phi^n - (-phi)^(-n)) / sqrt(5) will give an accurate answer (unlike the answer above of fib [n] = phi^n / sqrt(5) + 1/2), which breaks down at values of n greater than 70). Since you can calculate it directly, recursion is unnecessary.


1

The function get_sums_freqs returns an int, but when I call it, I do not assign the result to anything: get_sums_freqs (root, sum_freqs);. Is it ok to do it this way? Your uneasiness here probably arises from the difference between a Tree and a TreeNode. A TreeNode is an internal tree detail, which should be hidden from the user of the Tree. I should be ...


1

Your push class has a big chunk of code and does two lookups in the map. It can be replaced with one line of code: ++_tocc[item]; since operator[] will add a key/value pair if it does not exist, and default initialize the value (0 in the case of long int). pop will do three lookups if the item is found. This can be reduced to one by saving the result of ...


3

The rules of five/three/zero Usually, the existence of a single constructor indicates that the others should get implemented too (or explicitly forbidden/deleted). This is called the rule of five (or three, depending on the standard). However, there is only one member in your class, _tocc, and it has well-defined constructors for all the usual cases. Here, ...


1

This question is a bit too general. And I believe you are nitpicking but not for the reason you mentioned. In the code it first reads all 1000+ lines and stores them into a deque. Each line is a potential allocation and deque also allocates linearly in the number of objects (one allocation for each 4k bytes and each strings takes 32 bytes... surely these ...


0

using id = uint8_t; using buffer = std::vector<uint8_t>; static constexpr size_t header_length{sizeof(id)}; static constexpr size_t max_body_length{MAX_COMMAND_BODY_LENGTH}; Misspelt std::uint8_t and std::size_t here - that's a lurking portability bug (you're getting away with this because your current compiler is exercising its option to declare ...


3

While this thread already has many answers, one of which accepted, I would like to point out that the (naïve) recursive solution presented by OP has a much worse complexity than the iterative version. However, it is perfectly possible to split up the problem into a main function to be called by the user, and an internal helper function doing the work ...


2

The debate around recursive vs iterative code is endless. Some say that recursive code is more "compact" and simpler to understand.. In both cases (recursion or iteration) there will be some 'load' on the system when the value of n i.e. fib(n) grows large.Thus fib(5) will be calculated instantly but fib(40) will show up after a slight delay. Of course your ...


5

Neither nor! David Foerster's __fibonacci_impl has a matrix representation, where the matrix can be brought into a diagonal shape, evaluating to a difference of two exponential functions, where the absolute value of the latter one is less than one and so may be replaced by a rounding operator. const double sqr5 = sqrt(5); const double phi = 0.5 * (sqr5+1)...


2

Create a class Matrix Instead of having a vector of vectors, and have global functions that manipulate those, create a class Matrix and add member functions to it to manipulate matrices. You should probably also create overloads for arithmetic operations, so you can write things like auto matrix3 = matrix1 + matrix2; Have a look at existing C++ matrix ...


2

As other answers state, your iterative algorithm outperforms your recursive algorithm because the former remembers previous intermediate results (or at least one such result) while the latter doesn’t. Of course, one can write recursive algorithms that remember previous results. For Fibonacci numbers that’s simple enough since you only need to remember one ...


5

Remove unnecessary steps for (int i = 0; i < field_number - 1; ++i) { /*read discard*/ } double value; // read the needed value now This will make sure that it will not do unnecessary steps if the value needed is the very first one. Do not copy around I believe Rcpp would provide some way to access underlying char*. That can be used to construct std::...


5

Includes and using namespace std; You should not use #include <bits/stdc++.h> in any serious program, since this will pull in all standard library headers. Together with using namespace std; this brings a high chance of causing headache, because of possible name collisions and ambiguities. To get the standard library function you use, just include #...


6

Includes We have included <iostream> twice, but missed <cstdio>. Misspelt standard library identifiers std::printf, std::uint8_t and std::uint64_t are consistently misspelt. You might sometimes get away with this, as your standard library is allowed to add global-namespace versions of those identifiers; since it's not required to do so, you ...


2

Your code doesn't look that ugly! Here are some suggestions to make it better. Note that LeetCode makes significantly more mistakes than you when it comes to code quality; I have listed the suggestions for LeetCode-provided stuff too, so that you don't make the same mistakes when you are writing real code — you don't have to force LeetCode to accept ...


3

Is there a better way than these two methods?And are these methods complex? There are better methods, and although not that complex, few people would be able to develop such methods (such as Lucas sequence relations) on their own without relying on some reference. For the recursive version shown in the question, the number of instances (calls) made to ...


1

Consider absl::Span<T>. (or boost's) It may one day be replaced with std::span It's actually quite hard to make what you're asking for, despite this being extremely useful and desired for a very long time. However, with it, you can write: void ReadFromIntArray(absl::Span<int const> int_array); ReadFromIntArray({1,2,3}); ReadFromIntArray(std::...


27

Why do most people (on the internet) recommend using recursion because it's simpler and easier to write the program? Logically I thought that we should write it in a way that is fast and simple. This is a perceptive question. I wrote an article about exactly this topic in 2004, which you can read here: https://docs.microsoft.com/en-us/archive/blogs/...


2

By using that iterative solution, you are indirectly using Dynamic Programming (DP) Answer for question number 1: Recursion might be faster in some cases. For example, let's say you have a 2d road of size n * m. There are blockages in the road, so you can't pass through them. The objective is to check if there exists any path from the top-left corner to ...


4

In the recursive version of the code you don't need the function prototype unsigned long long fibonacci(unsigned long long n);. As you mentioned you shouldn't have the using namespace std; statement in the code. We can't answer Why do most people (on the internet) recommend using recursion because it's simpler and easier to write the program?(well ...


2

Honestly, .ini is an outdated format that is considered deprecated by Microsoft. Anyways, you don't actually need to write anything - you can use boost::ptree - property tree which can parse (read and write) ini, json, and xml formats. And it is fairly simple to use. The only reason I don't use it in my code is because I learned of it only recently. At any ...


5

This seems over-complicated. A simpler solution would use a std::map or std::unordered_map for the counting: std::unordered_map<int, std::size_t> counts; for (auto const &element: container) { ++counts[element]; } Instead of being tied to std::vector<int>, we could make the code much more flexible as a template, taking a pair of ...


2

int is redundant in this variable declaration: unsigned long int num See the properties table on this reference page: https://en.cppreference.com/w/cpp/language/types in this case, it is better to use ++n rather than n++ for better readability. ++n precisely describes what this imperative procedure tries to do: increment the value of variable n by 1 and save ...


1

I restructured the code into object oriented format. Incorporated changes suggested by pacmaninbw. #include <iostream> #include <vector> class replace_avg{ private: int n; int tmpIn; double f; double l; double av; std::vector<double> intputData; public: std::vector<double>...


6

We're missing some required header includes: #include <fstream> #include <memory> #include <regex> #include <string> #include <unordered_map> #include <utility> The code doesn't compile until we give IniNonCopyable an accessible default constructor: protected: IniNonCopyable() = default; Classes that inherit from ...


1

Always check input succeeded. std::cin >> n; std::cin >> arr[i]; In both these lines, we ignore all errors, and will produce the wrong output without any warnings, and happily return 0 from main(). This is bad for any program used as a processing step (e.g. driven by Make).


1

Even on a machine with relatively fast division, this algorithm (trial division) may be slower than a suitable sieve. Even more so, once you improve the program to accept multiple inputs. I encourage you to accept arguments rather than prompting for input. That said, I still reviewed the input code in main(): There's currently a bug that the program ...


2

Something to keep in mind is that this statement is truncating data: arr[j]=av; because it is assigning a double to an int. As was mentioned in a comment there are c++ container types that would be better than a old C style array, two of these are std::array<type, arraySize> and std::vector<type>. std::array is a fixed size and std::...


3

The code fails to build for me, but that could be platform and compiler compatibility. I attempted to build this on Windows 10 using CLion and Visual Studio 2019. Here are the errors and warnings I received: main.cpp(9): error C2672: 'irqus::typeek': no matching overloaded function found main.cpp(9): error C2893: Failed to specialize function template '...


3

Firstly, this can be rewritten as: if(((letter == 'E' || letter == 'R')&& secondLetter == 'Q' && DoFlip == false)|| ((letter == 'E' || letter == 'R') && secondLetter == 'A' && DoFlip == true) { //DoSmth here } According to distributive law - A && B || A && C = (B || C) && A In ...


4

One performance problem is the overhead of allocating and resizing a vector. Since you read the size up front, why not simply reserve() enough space? cin >> array_size; input_array.reserve(array_size); If you sorted the sequence, computing the median would become dead simple and very fast (O(1)). This may be a better approach. However, you ...


2

Formatting: Indent your code consistently. This makes it easier to understand and spot errors. Also, it attracts people here to read it at all. Your's is not a mess, but inconsistent still. Another rather obvious error is not to return a boolean for a function that gives you a true/false result. This is implicit documentation. If you return an int, you need ...


7

Not related to performance, but never ever do this: #include <bits/stdc++.h> using namespace std; For reference see these Q&A at Stack Overflow: Why should I not #include <bits/stdc++.h>? Why is “using namespace std;” considered bad practice?


4

Don't use using namespace std;. Since is_prime only has two possible return values, and given its name implies a binary result, it should return a bool, not an int. Then replace the return statements with the proper bool values. When checking for primes, you only need to go up to the square root of the number to check. This value can be computed once ...


4

Sorry, I probably should have used a "comment" for this, but it is way bigger than their limited size. In all fairness I did not read your code in full; I only scanned it. The thing that struck me as I was reading, is that this is incredibly hard to do well. Assuming a robust enterprise environment, you run into all kinds of issues that take lots of ...


3

If you're using Boost, one readable solution is: #include <boost/algorithm/string/split.hpp> #include <boost/algorithm/string/join.hpp> #include <boost/algorithm/string/classification.hpp> std::string reverseHost(const std::string& host) { std::vector<std::string> parts; boost::algorithm::split(parts, host, boost::...


3

To add to the other answer(s): The parameters to both getDeterminant and printMatrix should be const-ref instead of passed by-value. int dimension = getDimension(); should be const int dimension = getDimension(); Note that str = str + ','; and number = number + str[k]; could be rewritten to str += ',' and number += str[k], respectively, for potentially ...


2

I've noticed a few small things: You don't need to flush the stream every time you log something. A simple newline will do. A more suitable log stream provided by C++ would be std::clog. You may, however, need to flush this stream. Your formatting is inconsistent. You have spaces in places where you don't have spaces elsewhere, including but not limited to: ...


0

It's strange to use a signed type for length, but that appears to be imposed on you by the problem statement, so blame a poor specification for that. Despite the letters std, <bits/stdc++.h> is not a standard header, so you have a portability bug. Even where it exists, it brings in far more than you need, so get used to including just the headers you ...


1

Don't using namespace std; - especially not in a header, where it inflicts the harm on every source that includes the header. Prefer nullptr to NULL, because the former is more strongly typed. Use std::size_t for indexing, rather than int. When overloading operator[], it's usually necessary to provide two versions: int& operator[](std::size_t); int ...


3

To answer your specific question: To go with all unique pointers or should we use shared pointers in this case? You're absolutely correct to return std::unique_ptr values. This indicates that ownership of the pointed-to object is transferred to the caller. If you were to return std::shared_ptr, then that would imply that the factory retained a share of ...


1

You're trying to reinvent the wheel... This specific challenge was discussed in answers to this SO question: Read HowardHinnant's long answer, then read the final, pretty, constexpr answer here. @Snowhawk quoted the code there - now you have some references and the build-up to the final form of the code.


7

First, the big bug. Your Widget class has no virtual destructor! This means that every time you return a unique_ptr<Widget>, you're losing the information about what kind of widget needs destroying. Clang will actually diagnose this mistake for you. No matter what compiler you use, make sure you turn on warnings via -Wall -Wextra (or -W4 on MSVC). ...


0

Here are some suggestions you can use to improve your code. Formatting Insert a space before a { that starts a compound statement and does not go on a separate line; e.g., instead of int main(){, write int main() { or int main() { Place a space after a comma and after a control word (while, etc.): SendInput(1, &input, sizeof(INPUT)) and ...


2

This isn't really a code review so much as help on how to get this to compile. Array::Array(int *p = NULL, int s = 0){ default parameters go on the declaration, not the definition.* Array::Array& operator=(Array other){ Qualify the operator with Array::, not the return type. You're not returning an Array::Array**, you're defining the operator= member ...


2

In general when there is multiple options on a program like yours (on the section you manage the variable input), is desirable to make it more readable in order to avoid the multiples if statements. My recommendation is to use a map, or unordered_map with the input as first parameter and second a callback to the desire functionality. For example: for (int i ...


2

In addition to everything mentioned in Toby Speight's review: This program can use at least one additional class and probably more. The class that is obviously missing is Game. The class Game should be responsible for loading the game from a file, saving the game to a file and executing the game. The function createOriginalSave() should probably be broken ...


2

Overall I think the code is pretty good and your use of the more modern features of C++ looks OK. Still there are some things that could be improved. In no particular order I offer the following suggestions: The idea of a scoped_thread is good (also discussed in Effective Modern C++ item 37). Even if you want to implement your own std::thread replacement (...


3

You need to check for std::cin::eof() before std::cin.fail() (or simply !std::cin), because there's no point repeating the read infinitely if we've reached EOF. To demonstrate, just run the program with closed standard input, e.g. make 237153 ./237153 <&- The reading of the matrix's elements fails in a different way at end of stream: ./237153 <&...


2

Your code does effectively nothing but add a useless layer of indirection. There is functionally nothing different between a simple std::queue<std::function<void()>> queue; and your OOP for the sake of OOP hierarchy and queue of unique_ptrs to pure interface.


Top 50 recent answers are included