From: Marnen Laibow-Koser <marnen@...>
Date: 2010-02-13T02:10:24+09:00
Subject: Re: [noob] Problem with arrays

Sylvain Petit wrote:
> Hello,
> 
> I have a few issue using arrays.
> 
> I'm working with rubySDL to make a vertical shooting game. I have a
> ship, which had a collection of "fired_missile".
> The level have a collection of critters on screen.

I saw your later post that this is working, but I thought that since 
you're a newcomer, I'd just point a few things about your programming 
style that aren't so Rubyish...

> 
> My main loop is :
> 
>   def main
>     self.init_lvl
>     critest = Critters.new(@screen, SCREEN_W,SCREEN_H/2)

If each instance of this class represents one critter, then it would 
usually best to call the class Critter (in the singular).

>     @crit<<critest

I think most Rubyists use spaces around operators, for what that's 
worth...

>     while(@finished == false)

You don't need the parentheses or the "== false".  You probably just 
want "while !@finished".

Caveat: that will also return true if @finished is nil.

>       background = @screen.format.map_rgb(0, 0, 0)
>       @screen.fill_rect(0,0,@screen.w,@screen.h,background)
>       self.event_poll
>       @ship.move
>       @ship.draw
>       cur_crit = @crit
>       cur_crit.each { |c|

In general, "do...end" is used for multiline blocks.  The {} syntax is 
usually reserved for blocks on a single line.  It is *very* rare to use 
{} as you've used it here.

>       c.move
>       c.draw
>       self.check_firing(c)
>       }
>       @screen.update_rect(0,0,@screen.w,@screen.h)

That's a long loop!  Why not put it in a method of its own?  You might 
also want to put the cur_crit.each block in a method of its own.

>     end
>   end
> 
> If I understand my code well, for each critters in @crit collection (or
> cur_crit to be more accurate), I move the crit, then draw it, and to
> finish I call "check_firing" with the critters in parameter.
> 
> Here's the check_firing code :
> 
>   def check_firing(c)
>     critemp = []
>     if(@ship.firelist.length!=0)

Unnecessary.  If the length is 0, then the each in the next line will 
simply perform its block 0 times.

Now, if firelist could be nil, then you'll need to check for that.  "if 
@ship.firelist" would be sufficient.

>       @ship.firelist.each { |f|
>         if(f.collide_with?(c))
>         else
>           critemp<<c
>         end

Why do you have an empty if clause?  You could make this much more 
readable by reversing the conditional (and removing some parentheses):

if !f.collide_with?(c)
  critemp << c
end

>         @crit = critemp
>       }
>     end
>   end

Are you sure about your logic here?  The only thing that will be written 
to @crit is an array containing multiple copies of c.  No further 
information, just multiple copies of c.

Best,
--
Marnen Laibow-Koser
http://www.marnen.org
marnen@marnen.org
-- 
Posted via http://www.ruby-forum.com/.