patterncsharpMinor

Position update loop and checking collision

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

updatecollisioncheckinglooppositionand

Problem

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.

Solution

-
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 ...

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

-
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)

-
You don't actually use the index in this loop:

for (int i = 0; i 

- 
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.

- 
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.

Code Snippets

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

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

... your other code goes here ...

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

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

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

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))

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

Context

StackExchange Code Review Q#33938, answer score: 3

Revisions (0)

No revisions yet.