current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
1

I'm writing a laser connection game. This is my first game on Windows Phone, and I've never used C# and XNA before.

The part of my code that could be improved is Update() (where I make my sprites move).

public void Update(GameTime gameTime)
{
    TouchPanelCapabilities touchCap = TouchPanel.GetCapabilities();
    if (touchCap.IsConnected)
    {
        TouchCollection touches = TouchPanel.GetState();
        if (touches.Count >= 1)
        {
            Vector2 PositionTouch = touches[0].Position;

            for (int i = 0; i < ListDragObj.Count(); i++)
            {
                if (ListDragObj[i].Shape.Contains((int)PositionTouch.X, (int)PositionTouch.Y))
                {
                    //Permit to avoid several Obj to moove in the same time
                    if (!OnlyOneSelected(ListDragObj))
                    {
                        ListDragObj[i].selected = true;
                        TamponPosition = ListDragObj[i].Position;
                    }
                }
                else
                {
                    ListDragObj[i].selected = false;
                }

                if (touches[touches.Count - 1].State == TouchLocationState.Released)
                {
                    if (gm.Check_Tile((int)(PositionTouch.X / 10), (int)(PositionTouch.Y / 10))) 
                    {
                        ListDragObj[i].Update(PositionTouch);
                    }
                    else
                    {
                        ListDragObj[i].Update(TamponPosition);                                         
                    }
                }
            }
        }
    }
}

I'm looking for the selected Sprite (in the List ListDragobj), and then I move it. The last couple if/else is where I check collisions.

I was wondering how I could improve that position Update() method.

share|improve this question
    
touches.Count will give you the number of items, but the last item would be touches.Count - 1, hence the crashing –  SimonV Nov 6 '13 at 15:26
    
if the code doesn't work you should post this question on StackOverflow or Game Development –  Malachi Nov 6 '13 at 15:37
1  
Code is working. –  Gabson Nov 6 '13 at 15:43
1  
The last couple if/else is where I check collisions (it actually doesn't work fine, but that's another point) this could lead down a bunny trail to other issues in your code. get it functioning and then bring it back for review, or limit your review question to the parts of your code that work, if the code you want reviewed links {in any way} to the code that doesn't work, then this question is off-topic. –  Malachi Nov 6 '13 at 16:40
    
@Malachi It wasn't working but now its fine since SimonV told me how to correct it. –  Gabson Nov 7 '13 at 9:03

1 Answer 1

up vote 3 down vote accepted

  1. You can reduce some of the indent by checking for the negative condition and bailing out early. E.g.:

    TouchPanelCapabilities touchCap = TouchPanel.GetCapabilities();
if (!touchCap.IsConnected) { return; }

TouchCollection touches = TouchPanel.GetState();
if (touches.Count == 0) { return; }

... your other code goes here ...

  2. PositionTouch: Standard naming convention for local variables is camelCase. Also the names reads slightly weird - touchPosition would be better.

  3. On every iteration through your ListDragObj method you check if (touches[touches.Count - 1].State == TouchLocationState.Released). You can store that in a local variable before the loop:

    var touchReleased = touches[touches.Count - 1].State == TouchLocationState.Released;

for (...)
{
    ...
    if (touchReleased)

  4. You don't actually use the index in this loop: for (int i = 0; i < ListDragObj.Count(); i++). A more idiomatic C# way would be to use a foreach in this case:

    foreach (var dragObject in ListDragObj)
{
    ...
}

    And replace all uses of ListDragObj[i] with dragObject.

  5. This:

    for (int i = 0; i < ListDragObj.Count(); i++)
{
    if (ListDragObj[i].Shape.Contains((int)PositionTouch.X, (int)PositionTouch.Y))
    {
        //Permit to avoid several Obj to moove in the same time
        if (!OnlyOneSelected(ListDragObj))

    looks suspiciously like an O(n^2) algorithm - assuming that OnlyOneSelected iterates over the list and counts how many are selected. You should consider keeping track of how many items are selected in your class. This could be achieved by adding two methods:

    void SelectedObject(DragObj dragObject) { ... }
void DeselectObject(DragObj dragObject) { ... }

    which modify the selected property and adjust the selected count.

  6. This

    if (gm.Check_Tile((int)(PositionTouch.X / 10), (int)(PositionTouch.Y / 10)))

    looks like it computes a tile index from the position - assuming tiles are 10x10. If this assumption is correct then you should change Check_Tile to accept a position. It should then calculate the appropriate tile index by itself. Otherwise what happens if you ever want to change your tile size from 10x10 to something else? You would need to find every place in your code where you make this implicit assumption.

share|improve this answer
    
Thanks man, Since 1 month i have done some of these optimisation, i'm taking the overs too now :) –  Gabson Dec 2 '13 at 11:00
1  
+1 Great job, you really seem to take time to provide detailed answers for older unanswered questions (I guess it has to do with that recent meta discussion). –  Groo Dec 2 '13 at 12:58

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.