13

#include <iostream> #include <optional> #include <string> #include <string_view> #include <vector> #include <utility> #include <exception> #include <fstream> #include <cctype> #include <algorithm> I find it helps if I keep my includes in alphabetical order - that makes it easy to quickly check ...


10

Criticisms Datatypes Your data types own data they should not own. A Brewery has an identity, a location, and a set of beer it brews. It should not have a "distance to home" parameter, or a "visited" flag. Distance to who's home? Visited by whom? Usability haversine() is not a very usable function; it requires 4 parameters. It would make more sense ...


10

It looks like there is no need to recurse or loop at all. If the change is odd, it must include a 5-bill (special cases are changes of 1 and 3, which are impossible to make). The rest would be given by 10-bills (there will be change / 10 of them), and 2-bills (there will be (change % 10) / 2 of them). BTW, this is exactly what your code is doing, but in a ...


7

Whitespace There should be a blank line between two method definitions. Also, some blank lines inside of the methods would give the code room to breathe, and allow you to visually separate individual "steps" from each other, for example in your main method: int n = Integer.parseInt(args[0]); double step = Double.parseDouble(args[1]); Turtle turtle =...


7

I would like to add few improvements for better readability. You don't need an else statement; rather you should directly return it. For every if and else statement, even if it is a single line and Java allows it without braces, it's not preferable because of possible code bugs which may arise in future due to missing braces. Better to put (this.value <...


6

Before we got any further, a word of warning. The complexity is only valid in a particular computational model. The complexity of these algorithms is \$O(\log n)\$ only if the addition takes constant time. For large \$n\$ it is not the case. Fibonacci numbers grow exponentially with n. It means that the number of bits grows linearly. Now we are in the ...


6

The template seems reasonable, although inputArray is a misnomer. However, there is a major drawback that can be remedied with a single &: use call-by-reference instead of call-by-value (see guidelines): template<class T> inline long double Sum(const std::vector<T> &inputArray) { ... } Other than that it's perfectly fine for summing ...


5

Why would you do such a thing? I understand that you wrote the Arithmetic Expression Compiler, and perhaps want to show it off. But who would ever want to write a function as simple as a Fibonacci sequence generater using three programming languages (AEC, Intel assembly, and C++) mixed together, and type way more code than it would take in either C++ or even ...


5

The key parameter to frankenstein() does not appear to be used anywhere and can be eliminated. Use blank lines to split the code up into smaller logical chunks. It aides reading and understanding the code. in_dict is a misleading name, because it can be a list, dict, or something else altogether. In the for k, v in in_dict.items(): loop, in_dict[k] and v ...


5

Unnecessary check for dim < 0 Since dim is a std::size_t, it can never be negative, so this check is redundant. Reserve spaces in vectors If you know what the size of a vector is going to be up front, reserve() space for the elements to avoid unnecessary memory operations. Prefer emplace_back() over push_back() where possible In n_dim_vector_generator() ...


5

Make the desired container type a template parameter You are duplicating a lot of code, when the only thing that changes is the type of container that your are returning. You should make the container type a template parameter, for example like so: template<template<class...> class Container = std::vector, class T> requires (!is_iterable<T>)...


5

Template vs. function parameters Template parameters should normally be used for things that affect the type of the result, but not for other things. The template parameter times does not affect the type of the result, so it can just be a regular function parameter: template<template<class...> class Container = std::vector, std::size_t dim, class T&...


5

You are correct in your intuition that this isn’t really the best way to do this with Boost.Test. In fact, Boost.Test has about a half-dozen better ways to handle this, but I’ll focus on the one that I think will be of the most use for you. Normally I try to review the code as written, but in this case, it’s actually much easier to start from scratch and ...


5

Correctness Your code allows partial acronyms which doesn't seem right. "I'm seeking a clean and efficient way to determine whether one string is an initialism/acronym of another." recursive_split('aei', 'an example is this') # [('a', 'an'), ('e', 'example'), ('i', 'is')] Performance 1 A list comprehension being fed into a for loop is not ...


5

Inefficiencies Split/join You are calling s2.split() a lot. This seems wasteful. Moreover, you are calling ' '.join(s2.split()[1:]) to get a string containing everything but the first word of s2. So not only are you repeatedly splitting the string, you're also rejoining the splits. You're doing a lot of work, just to throw it away and redo it in subsequent ...


4

Variable names. Name your variables after what they do, no exceptions. As a sidenote, note that that you're relying on how integer division works in Java. Having said that, you can improve the code by being explicit. // As I've said, always name your variables and functions after // what they are doing. Don't be afraid to use longer names, // longer names ...


4

Repeated computations Part of the reason it is slow is that a lot of the computations are repeated many times. For example, if you diagram the calls for accelrate(3,3) you get something like this (where A is a call to accelrate and L is a call to lebniez): A(3,3) ...


4

Proper constraints Is it a good idea to set the type constraint like std::enable_if_t<std::is_arithmetic<T>::value, int> = 0, std::enable_if_t<std::is_arithmetic<T>::value, double> = 0>? It is not a good idea to constrain the value type to the arithmetic ones. It will only match primitive integer and floating point types. There ...


4

Using the right concept Indeed, Summable is the wrong concept to use if you want to check if the value type is such that you can calculate the maximum of two values. Have a look at the documentation of std::max(). In particular, it requires that the type is LessThanComparable. So you could create a concept to test that. There is already a concept std::...


4

Hi Nicholas and welcome to code review. A few things that I noticed in your code: The indentation isn't consistent. The closing bracket of menu isn't where it should be. The whole setAccountLocation method is one tab to far right. The readability of the code would be better if you'd fix that. In Eclipse, you can press Ctrl + Shift + F to automatically fix ...


4

Honestly, this is not a very good problem to do recursively, just pick another problem. Here's a better one: take in an integer, and print the sum of the digits. Some general feedback: ReverseString returns something, but you ignore its return value You make x, but it's a shallow copy of s -- modifying x and s do the same thing, and returning them would do ...


4

It's a great question, and the only improvements I can see are superficial ones: Your method has incomplete type hints. alphabet should be Iterable[str], and it should return -> Iterable[str] as well. PEP8 discourages multi-statement lines (your return). Otherwise, you should reconsider making this a recursive implementation. I see that you were careful ...


4

On a possible non-recursive solution, as a possible improvement (per Reinderien's answer). The general idea here is that if we build a tree of these strings in the obvious way*, then the lexicographical ordering is nothing but a DFS on this tree. *Let the root be the empty string, then its neighbours are the one character long strings for each character of ...


4

One approach to avoid string copying and improve the time complexity from \$\Theta(m^2)\$ to \$\Theta(m)\$ (\$m\$ is the output string length) for each output string is to store characters of strings in a list and modify it in-place. from typing import List, Iterable def lex_string_gen_recursive(cur_depth: int, ch_list: List[str], alphabet: Iterable[str]) -...


4

Everything that Toby said, plus: In operator<<, the output stream should not be called other. The variable name other should only be used in comparison functions between this and other. It should rather be called out or os. When I run the program, I get an error message that doesn't help me: $ ./cmake-build-debug/sudoku.exe sudoku.txt Invalid number of ...


4

Ensure init is only used once Since you pass the value of init to recursive calls to recursive_transform_reduce(), the value of init ends up being added potentially more than once to the output. For example: recursive_transform_reduce(std::vector<std::vector<int>>{{},{}}, 1, std::identity()) This will return 3 instead of 1. To fix it, instead of ...


4

Because this update seems primarily based on the changes to the concepts and requires clauses, that’s what I’m going to focus on. is_do_thingable is an anti-pattern. This is really an abuse of the idea of concepts: template<typename T> concept is_back_inserterable = requires(T x) { std::back_inserter(x); }; // ... template<typename T1, ...


4

This is fairly tight; I don't think there's much more performance to pull out without dropping into C. That said: possible_directions First, I would rename this to neighbors or successors; it's not really about direction but about the nodes in the graph that come next or are adjacent to your current node. Second, you can reduce the number of branches and ...


4

Compiler is g++ 4.2. Yikes. That compiler is ancient. I’ve met competitive programmers younger than that compiler. If you’re planning to do any serious C++ programming, you really should look into upgrading your toolchain, if possible. #define BIG unsigned long long int This is absolutely the wrong way to create a new type. First, there is a trend in C++ ...


4

To be honest, this is a bad way to solve the problem - recursion with strlen() is not a good idea. There was a recent discovery that Rockstar Games had their loading screens in GTA5 extended by several minutes due to their JSON parser calling strlen() on a string repeatedly. Instead, when handling strings in a recursive manner that requires knowledge of the ...


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