Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link
  1. Use auto-properties when you can like in RigidObject and set default values in the constructor.

     public double Mass { get; set; }
    
  2. I prefer to keep my private and protected members named with prefix underscore, and my method variables just lowercase. I saw you do some of this, and some not... it is good to keep this consistent.

  3. While I'm on naming and capitalization. Method names should be PascalCase not camelCase (start with a capital letter), this is especially true when the method is public

  4. So in a method like this, I would reccommend lowering the case of those xy's and using +=.

     public void AddForce(double x, double y)
     {
     forceX += x;
     forceY += y;
     }
    

    Same goes for later in your UpdatePhysics use -=.

  5. Unless you plan on adding more logic to this method, you can tune this down to a single statement. As you can see, if the first condition is false, the statement will be immediately broken and return false, only if true will the next condition be checked as this is the nature of && vs &. Now the result of second condition will be anded with true, which will simply keep it the same, and the value will be returned.

    Edit: Note that I also renamed this method to start with a verb, which is common practice, typically when something starts with is, it is a property or some state on an object.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle);
     }
    

    If you do indeed want to add more rigid objects that this could collide with, you can always append this statement to include them.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return 
     obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle) ||
     obj.GetType() == typeof(RigidRectangle) && isCollisionWith(obj as RigidRectangle);
     }
    
  6. As Malachi Malachi pointed out, you should indeed be using vectors for force direction. However, similarly to Malachi's point, you should be using a set value instead of a magic number. You should have some member something like this

     double _keyboard_force = 1.5F;
    

    Then in the case of your code when you apply the force.

     if (Keyboard.IsKeyPressed(Keyboard.Key.Left))
     MainCircle.AddForce( -1F * _keyboard_force , 0);
    
  1. Use auto-properties when you can like in RigidObject and set default values in the constructor.

     public double Mass { get; set; }
    
  2. I prefer to keep my private and protected members named with prefix underscore, and my method variables just lowercase. I saw you do some of this, and some not... it is good to keep this consistent.

  3. While I'm on naming and capitalization. Method names should be PascalCase not camelCase (start with a capital letter), this is especially true when the method is public

  4. So in a method like this, I would reccommend lowering the case of those xy's and using +=.

     public void AddForce(double x, double y)
     {
     forceX += x;
     forceY += y;
     }
    

    Same goes for later in your UpdatePhysics use -=.

  5. Unless you plan on adding more logic to this method, you can tune this down to a single statement. As you can see, if the first condition is false, the statement will be immediately broken and return false, only if true will the next condition be checked as this is the nature of && vs &. Now the result of second condition will be anded with true, which will simply keep it the same, and the value will be returned.

    Edit: Note that I also renamed this method to start with a verb, which is common practice, typically when something starts with is, it is a property or some state on an object.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle);
     }
    

    If you do indeed want to add more rigid objects that this could collide with, you can always append this statement to include them.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return 
     obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle) ||
     obj.GetType() == typeof(RigidRectangle) && isCollisionWith(obj as RigidRectangle);
     }
    
  6. As Malachi pointed out, you should indeed be using vectors for force direction. However, similarly to Malachi's point, you should be using a set value instead of a magic number. You should have some member something like this

     double _keyboard_force = 1.5F;
    

    Then in the case of your code when you apply the force.

     if (Keyboard.IsKeyPressed(Keyboard.Key.Left))
     MainCircle.AddForce( -1F * _keyboard_force , 0);
    
  1. Use auto-properties when you can like in RigidObject and set default values in the constructor.

     public double Mass { get; set; }
    
  2. I prefer to keep my private and protected members named with prefix underscore, and my method variables just lowercase. I saw you do some of this, and some not... it is good to keep this consistent.

  3. While I'm on naming and capitalization. Method names should be PascalCase not camelCase (start with a capital letter), this is especially true when the method is public

  4. So in a method like this, I would reccommend lowering the case of those xy's and using +=.

     public void AddForce(double x, double y)
     {
     forceX += x;
     forceY += y;
     }
    

    Same goes for later in your UpdatePhysics use -=.

  5. Unless you plan on adding more logic to this method, you can tune this down to a single statement. As you can see, if the first condition is false, the statement will be immediately broken and return false, only if true will the next condition be checked as this is the nature of && vs &. Now the result of second condition will be anded with true, which will simply keep it the same, and the value will be returned.

    Edit: Note that I also renamed this method to start with a verb, which is common practice, typically when something starts with is, it is a property or some state on an object.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle);
     }
    

    If you do indeed want to add more rigid objects that this could collide with, you can always append this statement to include them.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return 
     obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle) ||
     obj.GetType() == typeof(RigidRectangle) && isCollisionWith(obj as RigidRectangle);
     }
    
  6. As Malachi pointed out, you should indeed be using vectors for force direction. However, similarly to Malachi's point, you should be using a set value instead of a magic number. You should have some member something like this

     double _keyboard_force = 1.5F;
    

    Then in the case of your code when you apply the force.

     if (Keyboard.IsKeyPressed(Keyboard.Key.Left))
     MainCircle.AddForce( -1F * _keyboard_force , 0);
    
deleted 10 characters in body
Source Link
BenVlodgi
  • 4.3k
  • 2
  • 21
  • 47
  1. Use auto-properties when you can like in RigidObject and set default values in the constructor.

     public double Mass { get; set; }
    
  2. I prefer to keep my private and protected members named with prefix underscore, and my method variables just lowercase. I saw you do some of this, and some not... it is good to keep this consistent.

  3. While I'm on naming and capitalization. Method names should be PascalCase not camelCase (start with a capital letter), this is especially true when the method is public

  4. So in a method like this, I would reccommend lowering the case of those xy's and using +=.

     public void AddForce(double x, double y)
     {
     forceX += x;
     forceY += y;
     }
    

    Same goes for later in your UpdatePhysics use -=.

  5. Unless you plan on adding more logic to this method, you can tune this down to a single statement. As you can see, if the first condition is false, the statement will be immediately broken and return false, only if true will the next condition be checked as this is the nature of && vs &. Now the result of second condition will be anded with true, which will simply keep it the same, and the value will be returned.

    Edit: Note that I also renamed this method to start with a verb, which is common practice, typically when something starts with is, it is typically a property or some state on an object.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle);
     }
    

    If you do indeed want to add more rigid objects that this could collide with, you can always append this statement to include them.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return 
     obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle) ||
     obj.GetType() == typeof(RigidRectangle) && isCollisionWith(obj as RigidRectangle);
     }
    
  6. As Malachi pointed out, you should indeed be using vectors for force direction. However, similarly to Malachi's point, you should be using a set value instead of a magic number. You should have some member something like this

     double _keyboard_force = 1.5F;
    

    Then in the case of your code when you apply the force.

     if (Keyboard.IsKeyPressed(Keyboard.Key.Left))
     MainCircle.AddForce( -1F * _keyboard_force , 0);
    
  1. Use auto-properties when you can like in RigidObject and set default values in the constructor.

     public double Mass { get; set; }
    
  2. I prefer to keep my private and protected members named with prefix underscore, and my method variables just lowercase. I saw you do some of this, and some not... it is good to keep this consistent.

  3. While I'm on naming and capitalization. Method names should be PascalCase not camelCase (start with a capital letter), this is especially true when the method is public

  4. So in a method like this, I would reccommend lowering the case of those xy's and using +=.

     public void AddForce(double x, double y)
     {
     forceX += x;
     forceY += y;
     }
    

    Same goes for later in your UpdatePhysics use -=.

  5. Unless you plan on adding more logic to this method, you can tune this down to a single statement. As you can see, if the first condition is false, the statement will be immediately broken and return false, only if true will the next condition be checked as this is the nature of && vs &. Now the result of second condition will be anded with true, which will simply keep it the same, and the value will be returned.

    Edit: Note that I also renamed this method to start with a verb, which is common practice, typically when something starts with is, it is typically a property or some state on an object.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle);
     }
    

    If you do indeed want to add more rigid objects that this could collide with, you can always append this statement to include them.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return 
     obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle) ||
     obj.GetType() == typeof(RigidRectangle) && isCollisionWith(obj as RigidRectangle);
     }
    
  6. As Malachi pointed out, you should indeed be using vectors for force direction. However, similarly to Malachi's point, you should be using a set value instead of a magic number. You should have some member something like this

     double _keyboard_force = 1.5F;
    

    Then in the case of your code when you apply the force.

     if (Keyboard.IsKeyPressed(Keyboard.Key.Left))
     MainCircle.AddForce( -1F * _keyboard_force , 0);
    
  1. Use auto-properties when you can like in RigidObject and set default values in the constructor.

     public double Mass { get; set; }
    
  2. I prefer to keep my private and protected members named with prefix underscore, and my method variables just lowercase. I saw you do some of this, and some not... it is good to keep this consistent.

  3. While I'm on naming and capitalization. Method names should be PascalCase not camelCase (start with a capital letter), this is especially true when the method is public

  4. So in a method like this, I would reccommend lowering the case of those xy's and using +=.

     public void AddForce(double x, double y)
     {
     forceX += x;
     forceY += y;
     }
    

    Same goes for later in your UpdatePhysics use -=.

  5. Unless you plan on adding more logic to this method, you can tune this down to a single statement. As you can see, if the first condition is false, the statement will be immediately broken and return false, only if true will the next condition be checked as this is the nature of && vs &. Now the result of second condition will be anded with true, which will simply keep it the same, and the value will be returned.

    Edit: Note that I also renamed this method to start with a verb, which is common practice, typically when something starts with is, it is a property or some state on an object.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle);
     }
    

    If you do indeed want to add more rigid objects that this could collide with, you can always append this statement to include them.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return 
     obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle) ||
     obj.GetType() == typeof(RigidRectangle) && isCollisionWith(obj as RigidRectangle);
     }
    
  6. As Malachi pointed out, you should indeed be using vectors for force direction. However, similarly to Malachi's point, you should be using a set value instead of a magic number. You should have some member something like this

     double _keyboard_force = 1.5F;
    

    Then in the case of your code when you apply the force.

     if (Keyboard.IsKeyPressed(Keyboard.Key.Left))
     MainCircle.AddForce( -1F * _keyboard_force , 0);
    
added the need for verb start for method names
Source Link
BenVlodgi
  • 4.3k
  • 2
  • 21
  • 47
  1. Use auto-properties when you can like in RigidObject and set default values in the constructor.

     public double Mass { get; set; }
    
  2. I prefer to keep my private and protected members named with prefix underscore, and my method variables just lowercase. I saw you do some of this, and some not... it is good to keep this consistent.

  3. While I'm on naming and capitalization. Method names should be PascalCase not camelCase (start with a capital letter), this is especially true when the method is public

  4. So in a method like this, I would reccommend lowering the case of those xy's and using +=.

     public void AddForce(double x, double y)
     {
     forceX += x;
     forceY += y;
     }
    

    Same goes for later in your UpdatePhysics use -=.

  5. Unless you plan on adding more logic to this method, you can tune this down to a single statement. As you can see, if the first condition is false, the statement will be immediately broken and return false, only if true will the next condition be checked as this is the nature of && vs &. Now the result of second condition will be anded with true, which will simply keep it the same, and the value will be returned.

    Edit: Note that I also renamed this method to start with a verb, which is common practice, typically when something starts with is, it is typically a property or some state on an object.

     internal Boolean isCollisionWithCollidesWith(RigidObject obj)
     {
     return obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle);
     }
    

    If you do indeed want to add more rigid objects that this could collide with, you can always append this statement to include them.

     internal Boolean isCollisionWithCollidesWith(RigidObject obj)
     {
     return 
     obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle) ||
     obj.GetType() == typeof(RigidRectangle) && isCollisionWith(obj as RigidRectangle);
     }
    
  6. As Malachi pointed out, you should indeed be using vectors for force direction. However, similarly to Malachi's point, you should be using a set value instead of a magic number. You should have some member something like this

     double _keyboard_force = 1.5F;
    

    Then in the case of your code when you apply the force.

     if (Keyboard.IsKeyPressed(Keyboard.Key.Left))
     MainCircle.AddForce( -11F * _keyboard_force , 0);
    
  1. Use auto-properties when you can like in RigidObject and set default values in the constructor.

     public double Mass { get; set; }
    
  2. I prefer to keep my private and protected members named with prefix underscore, and my method variables just lowercase. I saw you do some of this, and some not... it is good to keep this consistent.

  3. While I'm on naming and capitalization. Method names should be PascalCase not camelCase (start with a capital letter), this is especially true when the method is public

  4. So in a method like this, I would reccommend lowering the case of those xy's and using +=.

     public void AddForce(double x, double y)
     {
     forceX += x;
     forceY += y;
     }
    

    Same goes for later in your UpdatePhysics use -=.

  5. Unless you plan on adding more logic to this method, you can tune this down to a single statement. As you can see, if the first condition is false, the statement will be immediately broken and return false, only if true will the next condition be checked as this is the nature of && vs &. Now the result of second condition will be anded with true, which will simply keep it the same, and the value will be returned.

     internal Boolean isCollisionWith(RigidObject obj)
     {
     return obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle);
     }
    

    If you do indeed want to add more rigid objects that this could collide with, you can always append this statement to include them.

     internal Boolean isCollisionWith(RigidObject obj)
     {
     return 
     obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle) ||
     obj.GetType() == typeof(RigidRectangle) && isCollisionWith(obj as RigidRectangle);
     }
    
  6. As Malachi pointed out, you should indeed be using vectors for force direction. However, similarly to Malachi's point, you should be using a set value instead of a magic number. You should have some member something like this

     double _keyboard_force = 1.5F;
    

    Then in the case of your code when you apply the force.

     if (Keyboard.IsKeyPressed(Keyboard.Key.Left))
     MainCircle.AddForce( -1 * _keyboard_force , 0);
    
  1. Use auto-properties when you can like in RigidObject and set default values in the constructor.

     public double Mass { get; set; }
    
  2. I prefer to keep my private and protected members named with prefix underscore, and my method variables just lowercase. I saw you do some of this, and some not... it is good to keep this consistent.

  3. While I'm on naming and capitalization. Method names should be PascalCase not camelCase (start with a capital letter), this is especially true when the method is public

  4. So in a method like this, I would reccommend lowering the case of those xy's and using +=.

     public void AddForce(double x, double y)
     {
     forceX += x;
     forceY += y;
     }
    

    Same goes for later in your UpdatePhysics use -=.

  5. Unless you plan on adding more logic to this method, you can tune this down to a single statement. As you can see, if the first condition is false, the statement will be immediately broken and return false, only if true will the next condition be checked as this is the nature of && vs &. Now the result of second condition will be anded with true, which will simply keep it the same, and the value will be returned.

    Edit: Note that I also renamed this method to start with a verb, which is common practice, typically when something starts with is, it is typically a property or some state on an object.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle);
     }
    

    If you do indeed want to add more rigid objects that this could collide with, you can always append this statement to include them.

     internal Boolean CollidesWith(RigidObject obj)
     {
     return 
     obj.GetType() == typeof(RigidCircle) && isCollisionWith(obj as RigidCircle) ||
     obj.GetType() == typeof(RigidRectangle) && isCollisionWith(obj as RigidRectangle);
     }
    
  6. As Malachi pointed out, you should indeed be using vectors for force direction. However, similarly to Malachi's point, you should be using a set value instead of a magic number. You should have some member something like this

     double _keyboard_force = 1.5F;
    

    Then in the case of your code when you apply the force.

     if (Keyboard.IsKeyPressed(Keyboard.Key.Left))
     MainCircle.AddForce( -1F * _keyboard_force , 0);
    
Source Link
BenVlodgi
  • 4.3k
  • 2
  • 21
  • 47
Loading
lang-cs

AltStyle によって変換されたページ (->オリジナル) /