19

Immutable objects are awesome. They are robust, predictable, and inherently thread-safe. Make the x, and y fields final, and change the operations to return the resulting Vector, for example: class Vector { private final int x; private final int y; public Vector(int x, int y) { this.x = x; this.y = y; } public Vector ...


19

Ok, so here we go for a few tips: Several methods such as AABB::contains and AABB::intersects do not modify the AABB instance they are called with. Therefore, you should const-qualify these methods to make sure that they can also be called in a const context. bool intersects(AABB other) const { /* ... */ } ^^^^^ As pointed by @...


16

The most important points have already been mentioned: Use double instead of int for the coordinates Make the fields private Concerning the recommendation to make the class immutable, I have to say that one has to really consider the possible application cases here. The Escape Analysis has significantly been improved in the recent Java versions. But for ...


16

I see a number of things that may help you improve your program. Since the existing review covered a lot of good points, this review will cover the parts not already mentioned. Use the correct form for main There are exactly two allowed version of main, according to the standard, and yours isn't one of them. This code has this: int main(int argc, const ...


15

Here is my refactoring: private void movePosition(Plant p) { xpos += Integer.signum(p.getXpos() - xpos) * DELTA_X; ypos += Integer.signum(p.getYpos() - ypos) * DELTA_Y; xpos = Math.floorMod(xpos, MAX_X); ypos = Math.floorMod(ypos, MAX_Y); } 1. Signum signum implements the sign function, which gives -1, 0 or 1 for negative integers, zero ...


15

Allocations can fail Don't use buffer until we know it's not null. (And no need to multiply by sizeof (char), since that's automatically a no-op). Please remember to free() it too (at least as an option). That reduces false positives with Valgrind's memory checker. I/O operations can fail Always check that your I/O succeeds. Consider this scenario: we ...


14

I don't see why you use a HashMap (or any Map) at all. What I'd do is: Get all the homes Filter all the homes to only get those which are unvisited Get the home that is closest The home that you get will either be visible, or it won't be visible. If it is not visible, there won't be any other visible homes either. Combining this with @Vogel's suggestion ...


13

Your code is performing integer division for the vector, which is bound to fail: public void div(Vector vector) { this.x /= vector.x; this.y /= vector.y; } In this case, if the input vector is say x = 10 and y = 10, and we are x = 19 and y=19, the result will be: this.x = 1; this.y = 1; which is counterintuitive. I would recommend either not ...


13

Good start, but here are a few comments: The name for this class should be Vector2D rather than Vector, because you cannot represent a 3D or nD vectors using this class. int is not right type for representing points; doubles fit more double x; double y; By default, fields in Java have package access and it's usually a horrible idea. You should hide your ...


13

I ran through your code, and agree that the output is pretty. I like the idea, and it is a good use-case for a number of exercises. Your code suffers though from a few issues that are awkward. First up, you have just one real method. Why? You should isolate discrete logic elements in to their own methods. That's the most apparent issue, but we'll get there, ...


13

You are inconsistent with your use of the this. 'suffix'. Nobody can agree on this, but you should try to be consistent without projects. I would remove the return values from every method which modifies Vector3: you have 2 copies of Min, and one is very confusing. All of the methods which happen to produce a vector (e.g. Abs, Min, Max) happen to ...


12

Wrong increment First, there is a bug in your main loop. This line: add ebx, 1 should be: add ebx, 4 because you are operating on 4 floats at a time. Right now, you are doing array elements [0..3] following by [1..4]. On my computer the program crashed because it had a problem doing an unaligned load. Simplify divide by 4 This part ...


11

I agree with @rolfl that you need to extract methods and use a decision table. From Code Complete 2nd Edition, Chapter 19: General Control Issues, page 431: Use decision tables to replace complicated conditions Sometimes you have a complicated test involving several variables. It can be helpful to use a decision table to perform the test rather than ...


11

Pre-computing the possible moves, and reducing the conditionals to a simple mathematical expression is a nice way to solve problems like this. Your code also suffers from repeating blocks, and duplicated logic: DRY - Don't Repeat Yourself. Solve that by doing function extraction. Finally, your code is full of magic numbers, which are prone to leak bugs.......


11

At a glance, OnScreen looks like a method that would raise some Screen event, as On[EventName] is, by convention, the name we use for methods that raise an event. Now, those would obviously return void, and yours is returning a bool - I think I would have it like this: public bool IsOnScreen(Vector2 point) The return statement is pretty intense in terms of ...


11

Faster sorting If the only purpose of the distance computation is for comparison, then you can speed it up by removing the call to sqrt(). You'll end up comparing the squares of the distances, which is equivalent to comparing the actual distances.


11

You should probably be using Numpy, although I don't know enough about your situation to comment any further. Assuming that you need to retain "pure Python", the following improvements can be made: Negation Replace (-1)* with - Generators Replace your for k in range(dimension-1): loop with orthogonal_vec = [ random.randrange(-1000,1000) for _ ...


10

Here is my take on it. Since the motions in x and y are independent, I define a class for motion in one direction. public class Coordinate { private final int worldSize; private final int stepSize; private int value; public Coordinate(int worldSize, int stepSize, int initialValue) { this.worldSize = worldSize; this.stepSize ...


10

Give your code some breathing room. I.e. ) { |a, b| instead of ){|a, b|. And there's no need to put everything on one line. The way your block works right now, it'd be better to store the result of pwn.split in a variable, instead of calling split twice. (There's also no need to abbrevate "pawn" as "pwn".) That being said, strings support array-like access, ...


10

You could try to use the following built-in methods instead: Rectangle.Contains(Point) instead of the OnScreen(Vector2 point). Rectangle.IntersectsWith instead of the OnScreen(Vector2 topLeft, Vector2 size). Thus your latter OnScreen method will become: public bool OnScreen(Vector2 topLeft, Vector2 size) { return this.Rectangle.IntersectsWith(new ...


10

ByDistance looks very ad-hoc. If you move dist to Point (where it really belongs) as a member function, ByDistance is not required at all, e.g: temp.erase(remove_if(temp.begin(), temp.end(), [&reference, distanceThreshold](Point& p){ return reference.dist(p > ...


10

Your operator overloads (operator== and operator()) should be const since they don't modify any data members. operator<< can just be a single line. It could also use "\n" instead of std::endl so that one isn't forced to have a buffer flush when invoking it (there is a different between the two). return o << "(" << p.x << ", " <&...


10

A comparison-function doesn't change either argument, so should take them by const&. You normally don't make comparison-operators member-functions to make sure they handle conversions equally for both arguments. If neccessary, that might mean making them friend-functions, whether defined inline or not. If you provide ==, you should also provide !=: They ...


10

This code is extremely hard to read and much longer than it needs to be. Use documented naming conventions for C#. Make small methods that do one thing well. public bool IsWall(int x, int y) { return this.MyWorld.GetObjectOnPosition(x, y) is Wall; } public bool IsValidForEditing() { int x = this.X; int y = this.Y; return !IsWall(x-1, y) || !...


10

A few issues: Functions defined in the class in the header are inline by default, so there's no need to use the keyword everywhere. Use std::copy, not std::memcpy. It's just as fast (if not faster), and doesn't have memcpy's trivially copyable requirement (currently there's no check to make sure the objects stored in the class are trivially copyable). ...


10

This is only a minor observation on top of what @Reinderien already wrote about your code. Writing function documentation like you did with def span_orthogonal(vec): '''vec represents n-dimensional vector''' '''Function spans an arbitrary vector that is orthogonal to vec''' does not work as expected. If you were to use help(span_orthogonal) you'...


9

Proposed solution Okay BoardPosition should be an immutable data structure like this: public class BoardPosition { public final int x; public final int y; public BoardPosition(int pX, int pY) { y = pY; x = pX; } } You should never specify which specific implementation of List you want but ...


9

I took your code and got it to compile with my version of Rust (rustc 0.13.0-dev (29ad8539b 2014-12-24 16:21:23 +0000)). I ran with your parameters (I hope I understood them correctly) and got an average time of 207.8 ms. I made a few changes here and there, but the main thing was changing TreeMap to HashMap. TreeMap doesn't exist anymore, only BTreeMap. ...


8

Performance (Without any profiling data, I'm guessing blindly here) You have a linked data structure (a tree), traversing the tree has poor cache locality and this is where your performance will likely suffer. Because every node could be a cache miss at worst. But please do measure this first, if you see that most of your time is spent getting the next ...


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