Return to Answer

• 1

• 5.3k
• 1
• 19
• 35

There are some things here to be improved:

1. Order your #includes alphabetically. This will facilitate checking whether all required #includes are there. Also, consider leaving a space between #include and <...> as this tends to be a coding style issue most programmers agree on. Also, you don't seem to use anything from future, so you should remove it.

2. //evil? yes. acceptable for readibility of post? yes. No, I disagree. You're putting your code up for review here, so you should be posting your real code and not some changed variant that will never actually see a compiler. Using using namespace std; or not can mean the differences between subtle bugs that may not manifest until some edge case occurs, and you'd probably want to be told about those in a code review. Also, in all honesty, writing std:: every few lines doesn't change the length of your code much at all.

3. It is discouraged nowadays to use #define for simple numeric constants. You could either use enum class here (or, if you don't have anything >=C++11, enum) or define constants.

4. Again, if you have C++11 or beyond available, you should prefer using to typedef, because it is much more readable and versatile.

5. Being explicit is good, but being too explicit is bad, too. unsigned int can be written as unsigned, which is most commonly used today. Ultimately, though, this is a personal style issue, so don't take this point as a required change.

6. int is not the right type for everything. For example, for (int i = 0; i<distanceGrid.size(); i++) might overflow because distanceGrid.size() might be bigger than int and, in any case, unsigned. The standard offers std::size_t for sizes, so you should make use of it where appropriate.

7. Don't use std::endl because it's horrible. Although it does insert an endline character into the output stream, it also flushes the underlying buffer which is very rarely required (and for that cases there is std::flush) and can seriously harm performance if you're doing a lot of IO. Just write '\n' instead, the underlying system takes care of converting it to the correct end-of-line sequence for the current OS.

8. Use member initialization lists where possible. Lines such as worldMinExtents = worldMinExt; should just be part of the member initialization list of your constructor.

9. Your code violates the Single Responsibility Principle. To be exact, distanceFieldGrid is doing not only its main purpose but also IO, which violates its responsibility. Extract the IO to a helper class/function so that other people can use distanceFieldGrid without getting extremely annoyed at standard output they don't want. The same is true for benchmarking: You shouldn't have a method doPathfind that measures time, but write a separate benchmark class/function, possibly utilizing a benchmark library that allows you to get more reliable results.

10. setPassable should take its first argument by const&. Currently, you're making a copy of gridPos every single time, but don't do anything with it that would change its contents. Taking the parameter by reference to const will remove that copy and thus speed up the code (probably).

11. nextNeighbours.emplace_back(std::piecewise_construct, forward_as_tuple(currentNeighbours[i].first - 1), forward_as_tuple(currentNeighbours[i].second)); is a lot of code that does remarkably little. std::piecewise_construct is very handy in some cases, but this is definitely not one of them. Just write nextNeighbours.emplace_back(currentNeighbours[i].first - 1, currentNeighbours[i].second), or just use push_back (std::pair<unsigned, unsigned> is not exactly what you would call expensive to copy).

12. Don't make assumptions about the container type everybody uses. Instead of taking a std::vector or a std::list or any other container as a function parameter, take a begin and an end iterator instead. Iterators enable the caller to use whatever container he wants while not putting a huge burden on the library implementer to ensure correctness for every available container type.

13. //a smaller grid to test correctness The keyword here is test. You should restructure your project and write actual tests and benchmarks for correctness and performance. The main function is not the right place to do any of those.

14. Since we are talking about restructuring your project: Nearly all of your code should be extracted into a header and an implementation file.

15. vector<uiVec2d>({ make_pair(3,4), make_pair(2, 4), /* many, many more elements */}) is terrible to read and verify. You should have a separate object definition, maybe even a dedicated generator function (in the test class this actually belongs to, of course).

You said that your code has a lot stripped out. However, if you really want us to help you, you should restructure your code into multiple smaller units and ask for review of each of these units alone. As is, your code may be big and bulky, but refactoring can help to limit the scope of functionality for certain classes and make effective code review possible.