Is this script correctly coded ?

[Chaos17]

Hello,

Can someone help me check if this script is correctly coded, please ?

A friend tried to teach how to script months ago and this is the result.

But I'm unable at the moment to meet him.

The purpose of this script is to help me call a sprite from a sprite sheet and to change it to my taste through a script command in an event. Yes, I'm more an eventer than a scripter...

class Chara < Game_Interpreter  def  initialize(id, type, nom, x=0, y=0, z=0, m)    sprite = Sprite.new        sprite.x = x    sprite.y = y    sprite.z = z        sprite.mirror = m    m = false        chemin = "Graphics/Units/#{type}/#{nom}"    sprite.bitmap = Cache.normal_bitmap(chemin)        w = sprite.width/3    h = sprite.height/5    sprite.src_rect.set(0, 0, w, h)        V[id] = sprite  end     def self.frame(id, frame)     V[id].src_rect.x = frame*V[id].width   end      def self.pose (id, pose)     V[id].src_rect.y = pose*V[id].height   end       def self.add(id, type, nom)      chemin = "Graphics/Units/#{type}/#{nom}"      bitmap = Cache.normal_bitmap(chemin)      rect = Rect.new(0, 0, bitmap.width, bitmap.height)      V[id].bitmap.blt(0, 0, bitmap, rect)      end   end

 

[Napoleon]

I'm not sure but

class Chara < Game_Interpreteris a bit odd. Why would you want multiple interpreters?

Instead I'd personally go for:

module Cha; module Sprite_Stuff  <config and code and variables here>end; endclass Game_Interpreter  def cha_add_stuff(..)    ...  endend

 

[Chaos17]

Thank you for your reply.

is a bit odd. Why would you want multiple interpreters?

Unfortunally as a noob I don't know why we did that.

 

[Napoleon]

I gave it another quick look and just a few quick tips (because I'm sorry but that script has many issues):

- I don't see any dispose() methods being called for the sprites+bitmaps unless you do that somewhere else. Possible memory leak.

- I have no idea what V[] is. I believe you created a global constant variable called V? Better use $chaos_my_bitmaps or something like that instead.

- You might want to store the sprites in the Scene or Game_Map (depending on your purpose) instead anyway.

- "m = false"? Why set it to false? It's a local variable that is never used anymore. I take it that you are not familiar with Ruby's variable scopes yet.

- You add instance methods to the Interpreter for something that is only specific to the sprites... You probably want to remove the "self.".

- You call blt(..) on a cached bitmap I believe? That's gonna be problematic/buggy sooner or later.

- Your last parameter is not optional for initialize(). I believe that you need to swap the parameters or use this instead: http://stackoverflow.com/questions/16322706/optional-arguments-with-default-value-in-ruby

- And at this point I stopped looking. It has many issues.

 

[cremnophobia]

However there is one thing that is well done: using string interpolation instead of String#+ or String#<<.

"Graphics/Units/#{type}/#{nom}"

is much better than (the too often used)

"Graphics/Units/" + type.to_s + "/" + nom.to_s

 

[Chaos17]

I gave it another quick look and just a few quick tips (because I'm sorry but that script has many issues):

- I don't see any dispose() methods being called for the sprites+bitmaps unless you do that somewhere else. Possible memory leak.

- I have no idea what V[] is. I believe you created a global constant variable called V? Better use $chaos_my_bitmaps or something like that instead.

- You might want to store the sprites in the Scene or Game_Map (depending on your purpose) instead anyway.

- "m = false"? Why set it to false? It's a local variable that is never used anymore. I take it that you are not familiar with Ruby's variable scopes yet.

- You add instance methods to the Interpreter for something that is only specific to the sprites... You probably want to remove the "self.".

- You call blt(..) on a cached bitmap I believe? That's gonna be problematic/buggy sooner or later.

- Your last parameter is not optional for initialize(). I believe that you need to swap the parameters or use this instead: http://stackoverflow.com/questions/16322706/optional-arguments-with-default-value-in-ruby

- And at this point I stopped looking. It has many issues.

A lot of the flaws that you pointed are managed outside of the script through script calls because like I said before I'm an eventer.

Though, that's sad to see a lot of problem on a such a small script, that's makes me depressed since I don't have your knowledge to fix all of these problems.

Anyway, I thank you to have took your time for me. I appreciate it.

 

[Napoleon]

Maybe I was a bit too harsh. Well it is not "wrong" as long as it doesn't cause any bugs, unnecessary incompatibilities, performance drains/memory leaks or side-effects imo.

So if your script works and if the events handle everything outside of it (which you want) then aside from the "blt() on the cached bitmap" and the "non-optional last parameter" this script is solid I guess. Sure it has some unnecessary and untidy code and if you forget to manually dispose a sprite inside an event you could  have a major problem. But whether or not a script should 'fail-safe-automatically-dispose' all sprites when switching maps or unloading scenes for example is all a matter of opinion and circumstances. But if you are okay with that then your script is pretty solid. Coding is a matter of opinion&standards anyway.

And Ruby in RGSS doesn't seem to trigger warnings when altering the contents of a constant variable (technically they are not constants). But because your constant contains a reference to a hash you technically don't alter the constant anyway (the reference to the hash itself is never altered).

A = 'a'A += 'v'p A # 'av' # <<< Should raise a dynamic constant assignment error. But in RGSS it doesn't...B = {}B[:a] = 0 # <<< This always works in Ruby because B itself is not altered because the constant B is nothing but a pointer. The hash itself is not inside B I believe.I don't consider either of the above 'proper' coding but they both work perfectly. The end-user would call it good code (it works) but a programmer would call it bad code.

 

[Chaos17]

It's okay, don't worry you weren't harsh.

It just too bad that I don't know much about Ruby that I can't reply you properly about your questions.

Also, I'm always interest in explanations about RGSS. It always amaze me to discover some stuff  about it.

Again, thank you for taking your time for me