2
\$\begingroup\$

This is a custom MyActivity class with two custom views (bar and a ball):

public class MyActivity extends Activity {

    // set class variables
    RelativeLayout content;
    public Ball view;
    Bar bar;
    Handler handler;
    BounceLoop thread;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_my);
        content = (RelativeLayout) findViewById(R.id.content);
    }

    @Override
    public void onWindowFocusChanged(boolean hasChanged) {
        view = new Ball(this);
        bar = new Bar(this);
        Random rand = new Random();
        int min = 50;
        int max = content.getWidth();
        int randNum = rand.nextInt(max-min+1) + min;
        view.setPosition(randNum, min);
        bar.setY(content.getHeight() - 220);
        bar.setMaxWidth(content.getWidth());
        content.addView(bar);
        content.addView(view);
        thread = new BounceLoop();
        thread.start();
        handler = new Handler() {
            public void handleMessage(Message msg) {
                view.setPosition(msg.arg1,msg.arg2);
                view.invalidate();
            }
        };
    }
    @Override
    protected  void onDestroy() {
        super.onDestroy();
        content.removeAllViews();
    }
    @Override
    protected  void onResume() {
        super.onResume();
    }

    @Override
    protected void onPause() {
        super.onPause();
    }

    class BounceLoop extends Thread {
        public boolean running;
        int speed = 1;
        boolean reachedWidth, reachedHeight;
        int x = view.x,y = view.y, maxWidth = content.getWidth(), maxHeight = content.getHeight();
        int radius = 40;

        public void onPause() {
            running = false;
        }
        public boolean isCollision(int x, int y) {
            if( x > bar.x && x < bar.x + bar.getBarWidth()) {
                if(y >= bar.getY() - 50) {
                    return true;
                }
            }
            return false;
        }
        public void run() {

            running = true;
            Log.d("prevPos", String.valueOf(x + " : " + y));



            while(running) {
                if(reachedWidth) {
                    x -= speed;
                } else {
                    x += speed;
                }

                if(reachedHeight) {
                    y -= speed;
                } else {
                    y += speed;
                }

                if( x > maxWidth - radius) {
                    reachedWidth = true;
                } else if ( x < 0 + radius) {
                    reachedWidth = false;
                }

                if( y > maxHeight - radius || isCollision(x,y)) {
                    reachedHeight = true;
                } else if ( y < 0 + radius) {
                    reachedHeight = false;
                }
                try {
                    Thread.sleep(1);
                    Message msg = new Message();
                    msg.arg1 = x;
                    msg.arg2 = y;
                    handler.sendMessage(msg);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
    }
}

How do I re-construct it better and how can I put thread class outside? I really don't like having the thread class inside the activity class. The code seems to be working for now, but I really want to make it better! What can I do about it?

\$\endgroup\$
  • \$\begingroup\$ You could move remove the dependency from Thread on the your view & content object. Instead pass these as contructor parameters to your thread an use them in the run method. The Handler is used correctly to decouple the thread from your main class - pass this handler also as a constructor argument to your BounceLoop \$\endgroup\$ – 6ton Oct 14 '14 at 19:38
  • \$\begingroup\$ thanks, i'm am making a pong game, but I am still learning and making step by step. I just want to do things slowly and structured. Do you have advices? Thanks! \$\endgroup\$ – Кристиян Кацаров Oct 14 '14 at 19:38
  • \$\begingroup\$ I understand, after I pass the arguments, how do I send handler messages to the main activity ( ui thread ) ? \$\endgroup\$ – Кристиян Кацаров Oct 14 '14 at 19:39
  • \$\begingroup\$ If i make the thread in a separate file, I cannot send handler messages. \$\endgroup\$ – Кристиян Кацаров Oct 14 '14 at 19:42
1
\$\begingroup\$

How do I re-construct it better and how can I put thread class outside? I really don't like having the thread class inside the activity class.

You put the "thread" class outside by first creating more classes to isolate the different parts of your code.

The first thing to realize is that you have a ball physics engine; it's job is to figure out where the ball should be next. The physics engine needs to know where everything is, and how fast it is going, and so on; but it doesn't really care where that information comes from, or who is watching it.

It especially doesn't need to know that it is running in a thread, or how often that thread should loop.

It should know how to share the current location of the ball with other code, but it doesn't need to know anything about how that information is being used.

The basic skeleton might look like

class BallPhysicsEngine {
    public interface BallListener {
        void ballAt(int ballX, int ballY);
    }

    public void moveBall(BallListener listener) {
        // Do all the math to figure out where the ball is now
        ...

        // Then tell the listener where it is
        listener.ballAt(x,y);
    }
}

Now, you still need to run moveBall at the right interval. You don't implement a Thread for that, though (most of the time, implementing your own thread is overkill). Instead, you implement a Runnable

class BallPhysicsRunner implements Runnable {
    private final BallPhysicsEngine engine;
    private final BallPhysicsEngine listener;

    // Volatile so you can be stopped by another thread!    
    public volatile boolean running = false;

    public void run () {
        running = true;

        while(running) {
            engine.moveBall(listener);
            try {
                Thread.sleep(1);
            } catch (InterruptedException e) {
                // InterruptedExceptions are for implementing Cancellation!
                running = false;
            }
        } 
    }
}

onWindowFocusChanged now looks a little bit more complicated, because we want to make all the data transfer to the engine explicit.

public void onWindowFocusChanged(boolean hasChanged) {
    Random rand = new Random();

    view = new Ball(this);
    content.addView(view);
    int min = 50;
    int max = content.getWidth();
    int randNum = rand.nextInt(max-min+1) + min;
    view.setPosition(randNum, min);

    bar = new Bar(this);
    content.addView(bar);
    bar.setY(content.getHeight() - 220);
    bar.setMaxWidth(content.getWidth());


    BallPhysicsEngine engine = new BallPhysicsEngine(...);

    BallPhysicsEngine.Listener listener = new BallPhysicsEngine.Listener() {
        public void moveBall(int x, int y) {
            view.setPosition(x,y);
            view.invalidate();
        }
    };

    Runnable engineRunner = new BallPhysicsRunner(engine, listener);

    // Even better would be to use an ExecutorService!
    new Thread(engineRunner).start();
}

Recommended reading for more advanced versions of the game engine: Glenn Fiedler on time steps.

\$\endgroup\$
  • \$\begingroup\$ Awesome! That's what I wanted! Do you know where I can learn more about structuring things correctly? Thanks! \$\endgroup\$ – Кристиян Кацаров Oct 17 '14 at 17:44
1
\$\begingroup\$

I don't have time to do a full review, but here are some pointers.

  • Don't use raw access to members (view.x), but always use getters/setters instead. It has been a Java convention for about 15 years. Also, you should declare all your members as private.

  • Don't do many assignements on a single line (int x = view.x,y = view.y, ...). Again, it does work, but the Java convention is to have one assignment per line.

  • Don't have Ball being a member of MyActivity and MyActivity being a member of Ball. That is spaghetti code.

  • Avoid using Thread in Android, or Java. There are cleaner and less dangerous patterns for working with concurrent code. For Android, you should look into AsyncTask; for example this. You can probably find a more appropriate pattern if you look harder.

  • I haven't touch Android for a while, but I don't think onWindowFocusChanged is the usual callback where everything is started.

\$\endgroup\$
  • \$\begingroup\$ i understand, but the last point... how do I get sizes of a view inside onCreate then ? (without listeners, it's not a good way too) \$\endgroup\$ – Кристиян Кацаров Oct 14 '14 at 22:15
  • \$\begingroup\$ Also i don't understand the third point, that ball is member of myactivity and opposite, how did I achieve that ? \$\endgroup\$ – Кристиян Кацаров Oct 14 '14 at 22:32
  • \$\begingroup\$ About the last point, as I said, I don't remember. Maybe someone else can comment on that. \$\endgroup\$ – toto2 Oct 14 '14 at 22:41
  • \$\begingroup\$ For the third point, you have the member view = new Ball(this), where this is itself the container of the member, MyActivity. \$\endgroup\$ – toto2 Oct 14 '14 at 22:43
  • \$\begingroup\$ how should I fix this with the ball, I saw it this way in a tutorial. It is the same with the bar as well :) \$\endgroup\$ – Кристиян Кацаров Oct 14 '14 at 22:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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