Hot answers tagged

13

In addition to the suggestions given by 1201ProgramAlarm's answer, I have the following suggestions. First, your (over)use of RTTI seems overkill here. Even dynamic polymorphism is not necessary. A simple class is enough: enum class Species { cat, dog }; struct Animal { Species species; std::string name; }; (You are missing #include <string&...


11

To supplement the other reviews, here are some other things you might improve. Use override where appropriate When a virtual function is being overridden, it should be marked override to allow catching errors at compile time. See C.128. Make sure all paths return a value The setType routine claims it returns an int but it does not. That's an error that ...


11

First off, you should avoid using namespace ::std. In Animal, getOrder and getType should be const (int getOrder() const). Since they're defined within the class definition, you don't need to include the inline keyword. Similarly, getters in the derived classes should be const. It is rarely ever necessary to use the this keyword, and the places where you ...


6

Lots of good comments already given. I'll just point out that you should most likely not be writing your own math primitives. It's easy to get wrong, it takes time away from actually creating what you're trying to create, you'll tear your hair out fixing hard to spot bugs and your code (contrary to what most people who write their own math primitives seem ...


6

I would probably do this by reading the words into a vector of strings, then rather than reversing the order, just traverse the vector in reverse order: #include <iostream> #include <vector> #include <string> #include <iterator> #include <algorithm> int main() { std::vector<std::string> words { std::istream_iterator&...


6

Your algorithm is supremely inefficient. Moving a word to the front moves all the other characters to the back, resulting in a quadratic algorithm. In addition to that, you repeatedly recalculate the length of the null terminated string. As an aside, the standard library provides std::rotate() for moving part of a sequence from the end to the beginning, no ...


5

Here are some suggestions. Non conformance Many of your functions are marked constexpr. This is non-conforming. Per [constexpr.functions]: This document explicitly requires that certain standard library functions are constexpr ([dcl.constexpr]). An implementation shall not declare any standard library function signature as constexpr except for ...


5

General - it might be better to create a matrix class and your own vector class in a namespace. Allow The Tools to Help You Improve the Code There are compiler settings that can help you improve your code, these can be specific the the c++ compiler you are using or they can be common. A common c++ compiler switch is -Wall which indicates a errors and ...


4

In addition to other answers: you're using inline a lot. Do not. Modern compilers (almost all starting from year 2000) don't use it for optimization anyways: they are already clever enough to do so without hints when possible. These days inline have another meaning in some contexts (when function is defined in many translation units), but in your situation ...


3

I see some things that I think could help you improve your code. Don't abuse using namespace std Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Eliminate global variables where practical Having routines dependent on global variables makes it that much more difficult to understand the logic and ...


3

Serious issues with this: Memory Management. Not using RAII to lock/unlock Memory Management Don't pass pointers it does not indicate owners. Always wrap pointers in a smart pointer. Have a look at std::unique_ptr. But for types like Data there is no need to use pointers. Simply use Data as the object type (not Data*). RAII Look up the concept of RAII. ...


3

I'm not sure what your optimizations are doing(comments would be nice), but I did notice several inefficiencies: Never include a calculation in the limit test in a for loop. It get's re-calculated on every iteration. In this case set the limit to sqrt(limit). the calculation is kind of expensive but it's done only once. The same goes for the iteration ...


3

I suggest not codifying the derived types suggested in the type. Only note the ultimate base, size and alignment. Thus you are open to later change. Add a templated alias to get the proper type from base plus candidates. Assure that no over-sized object is ever assigned in PolyUnionSize, probably best using SFINAE. No need to defer to the user. Don't assume ...


3

Self-assignment may be expensive, but it should be a no-op instead of UB. I suggest copy-and-swap. Omitting move-semantics will most certainly cost you. Your naming of the internal class suggests all callables are lambdas. Not true! You cannot store any move-only callables. Admittedly std::function is also crippled in that respect. Just throw an exception if ...


2

Action.h The Action class only contains public static data members. While it's not illegal, a class might not be the best way to go about it. Consider using struct, which is the same as a class but has publicas the default access specifier. Or even better, don't use a class or struct and instead, wrap it inside a namespace, which seems perfect for this kind ...


2

Your code isn't functional, neither in the sense of working properly, nor in the sense of respecting the principles of functional programming. To parse a file, you need a way to consume the file. How would you do it with your function? csvStringView fh = readFile(filepath.csv); csvToken tok = csvFirstMatch(fh); // ??? You could imagine to compute a new ...


2

Here are some things that may help you improve your code. Use consistent formatting The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently. Use more whitespace Lines like this one: for(int i=0;i<5;i++){ are easier for most humans to read and understand with more whitespace ...


2

I'll assume in 2019 that C++17 is available. All other answers seem to be using two queues, one for each type of animal, but I think it kinda defeats the purpose that the FIFO behaviour has to be on the whole set of animals. With two queues, the FIFO has to be implemented by an _order stored with the animal, which mixes the data with the algorithm. Once the ...


1

I'm sorry to be so straightforward: This is madness! Your code seems to indicate a sincere lack of several language features. Pointer to const You define MEM to be of type uint8_t*, but it actually points to values of const uint8_t. To account for this difference you cast the const away in both constructors. If you'd ever (accidentally) write to the ...


1

If it is important to execute the reversing in place, here is an example code (which implements the algorithm mentioned in https://codereview.stackexchange.com/a/224037): #include <stdio.h> #include <stdlib.h> #include <string.h> #include <ctype.h> static void reverse_chars(char *s, int n) { int n2 = n / 2; for (int i = 0; i &...


1

This quite simple task. Just find words in reverse order and put them in new string. Code should be quite simple: std::string reverse_words(std::string_view s) { std::string result; result.reserve(s.size()); while(!s.empty()) { auto i = s.rfind(' '); result.append(s.begin() + i + 1, s.end()); if (i == std::string_view::npos)...


1

We're in C++17, so we can use std::aligned_storage_t: typename std::aligned_storage_t<Size, Align> storage = {}; (I added the initializer to pacify g++ -Weffc++; I also added virtual ~Base() = default; for the same reason). One thing that breaks is that I can't assign a B to the object unless the base is explicitly listed as one of the Ds.... ...


1

It may well be more efficient to compute std::sqrt(max) once upfront than to multiply n * n every time around the loop. Removing an element by value from the set is O(log n), where n is the set size at the time. This will likely make this method slower than writing to elements in a fixed-size storage (such as a std::vector that's never resized), and ...


1

#define int long long Not only is redefining int a bad idea, but using long long should be reserved for legacy code. Use <cstdint> and (given the ranges used in this code) std::uint_fast32_t. if (limit > 2) cout<<"2 "; if (limit > 3) cout<<"3 "; ... 21 lines ... for(int i=sieve._Find_first();i< ...


1

Here are a few things you could try... Including those mentioned by @tinstaafl I think that you can avoid a lot of the computations. I had a brief look at the Wikipedia page, it seems that you are otherwise properly following the algorithm. #include<bitset> #include<vector> #include<iostream> #include<algorithm> #pragma GCC target (...


1

A small observation on the includes and namespaces: We have #include <cstddef>, but then use size_t in the global namespace, which is not portable according to the standard. We should be using std::size_t instead. In the other file, we include the C compatibility header <string.h> - prefer to include <cstring> in new code, so that the ...


1

It might be overkill for just the task at hand, but I've found it useful to write an overload of operator>> to read and match a string literal: std::istream &operator>>(std::istream &is, char const *s) { while (*s && *s == is.peek()) { ++s; is.ignore(1); } if (*s != '\0') is.setstate(std::...


1

You don't check whether the input is invalid. Try to extract useful well-named functions, like int days(int month, int year) and bool is_leap_year(int year). You can extract the numbers directly from std::cin, the colon will be left behind. And the colon can thereafter be extracted like any other single character. As an aside, using scanf() would probably be ...


1

No time to read through all the code for now. So just some random observations. In Coordinates.h: Include as less as poissible in headers: You include iostream but it is only needed in Coordinates.cpp. Generally you should include as less as possible in header files. If you include iostream in the header every class which includes Coordinates.h also ...


1

L. F. gave a good review, but there is more: Exceptions are not for programmer errors. That's what asserts are for. Modularise your code: Extract down_heap() and up_heap() as free functions, making the algorithm available for anyone wanting to manipulate a heap. They should accept an iterator-range and a comparator, with the default being std::less<>(...


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