Feedback wanted on code style

BlitzMax Forums/BlitzMax Beginners Area/Feedback wanted on code style

Juxta

(Posted 2009) [#1]

Hi guys,

I grabbed a copy of BlitzMax yesterday and have been playing around with it. I think I'm slowly coming to grips with how it works, but I want to make sure I'm using good coding practice. If you have the spare time I'd appreciate some feedback on this code. Am I using good style? Am I using the most appropriate approach? etc.

Any and all feedback appreciated.

Here's the code:

SuperStrict

'Constants For window dimension others
Const SCREEN_WIDTH:Int = 1024
Const SCREEN_HEIGHT:Int = 768
Const MAX_COLOURS:Int = 255

'set window dimensions
Graphics SCREEN_WIDTH, SCREEN_HEIGHT, 0 'Set window size

'basic "star" object has positon on screen and a colour
Type TStar
   Field x:Int, y:Int 'position variables
   Field R:Int, G:Int, B:Int 'colour variables

   'randomly set positon based on window dimensions
   'and randomly set colour upon creation
   Method New()
      Local screenWidth:Int = GraphicsWidth()
      Local screenHeight:Int = GraphicsHeight()
      Self.x = Rnd(screenWidth)
      Self.y = Rnd(screenHeight)
      Self.R = Rnd(MAX_COLOURS)
      Self.G = Rnd(MAX_COLOURS)
      Self.B = Rnd(MAX_COLOURS)
   EndMethod
EndType

'a "star field" object that has its own draw and "movement" methods
Type TStarField
   Field sf:TStar[]
   Field starCount:Int
   Field speed:Int = 1 'movement speed starts at 1 by default
   Field counter:Int 'variable used to count through loops

   Method New()
      starCount = GraphicsWidth() 'width of screen used as a 'good' approx of number of stars
      sf = New TStar[starcount] 'initialise the array
      'populate the field with "stars"
      For counter = 0 To (starCount - 1)
         sf[counter] = New TStar
      Next
   EndMethod

   'draws all the stars to the current buffer
   Method DrawField()
      For counter = 0 To (starCount - 1)
         SetColor(sf[counter].R, sf[counter].G, sf[counter].B)
         Plot sf[counter].x, sf[counter].y
      Next
   EndMethod

   'sets the speed of the movement
   Method SetSpeed(newSpeed:Int)
      speed = newSpeed
   EndMethod

   'move the starfield up the screen
   Method MoveUp()
      For counter = 0 To (starCount - 1)
         'move point to bottom of screen first if it is already at the top
         If sf[counter].y <= 0 
            sf[counter].y = (GraphicsHeight() - 1)
         Else
            sf[counter].y = sf[counter].y - speed         
         EndIf
      Next
   EndMethod

   'move the starfield down the screen
   Method MoveDown()
      For counter = 0 To (starCount - 1)
         'move point to top of screen first if it is already at the bottom
         If sf[counter].y >= (GraphicsHeight() - 1) 
            sf[counter].y = 0
         Else
            sf[counter].y = sf[counter].y + speed         
         EndIf
      Next
   EndMethod

   'move the starfield right on the screen
   Method MoveRight()
      For counter = 0 To (starCount - 1)
         'move point to far left of screen first if it is already at the far right
         If sf[counter].x >= (GraphicsWidth() - 1) 
            sf[counter].x = 0
         Else
            sf[counter].x = sf[counter].x + speed         
         EndIf
      Next
   EndMethod

   'move the starfield left on the screen
   Method MoveLeft()
      For counter = 0 To (starCount - 1)
         'move point to far right of screen first if it is already at the far left
         If sf[counter].x <= 0 
            sf[counter].x = (GraphicsWidth() - 1)
         Else
            sf[counter].x = sf[counter].x - speed         
         EndIf
      Next
   EndMethod
EndType

'Create the starfield object
Global starfield1:TStarField = New TStarField

'main loop

While Not KeyHit(KEY_ESCAPE)

   Cls

   'print instruction text to screen
   SetColor(255,255,255)
   DrawText "Hit [Esc] key to exit", 10,10
   DrawText "Press 1, 2 or 3 to change speed", 10, 25
   Local speedString:String = "Current Speed is: " + String.FromInt(starfield1.speed)
   DrawText speedString, 10, 40 
   
   'check for and change speed
   If KeyDown(KEY_1) Then starfield1.SetSpeed(1)
   If KeyDown(KEY_2) Then starfield1.SetSpeed(2)
   If KeyDown(KEY_3) Then starfield1.SetSpeed(3)
 
   'check for movement key press
   If KeyDown(KEY_UP) Then starfield1.MoveUp()
   If KeyDown(KEY_DOWN) Then starfield1.MoveDown()
   If KeyDown (KEY_LEFT) Then starfield1.MoveLeft()
   If KeyDown (KEY_RIGHT) Then starfield1.MoveRight()
   
   'draw the starfield
   starfield1.DrawField()

   Flip

Wend

End

Brucey

(Posted 2009) [#2]

Looks fine.
I tend to use a lot more "whitespace"... blanks lines etc... to make it easier to read.
I also don't use single line If statements, and always use "Then" - but everyone does If's their own way :-)

Instead of :

For counter = 0 To (starCount - 1)

you can use

For counter = 0 Until starCount

:-)

Juxta

(Posted 2009) [#3]

Thanks Brucey :)

I like the until option. I must have missed it in the doco.

Yeah the 'if' syntax is a bit open to preference - the other option I was considering was along the lines of:


If KeyDown(KEY_UP) 
   starfield1.MoveUp()
ElseIf KeyDown(KEY_DOWN) 
   starfield1.MoveDown()
ElseIf KeyDown (KEY_LEFT) 
   starfield1.MoveLeft()
ElseIf KeyDown (KEY_RIGHT) 
   starfield1.MoveRight()
EndIf

but I wasn't sure about readability.

jsp	(Posted 2009) [#4]

Looks all very well, although i would not use the GraphicsWidth() , GraphicsHeight() everywhere in the code, that makes it for my taste too dependent.

Your 'optional' if statements have a different logic! You can't move diagonal with that.

Juxta

(Posted 2009) [#5]

Thanks for the feedback jsp.

I'm not sure what you mean by too dependent. I was using GraphicsWidth() etc to make it less dependent on pre set constants and work for whatever screen/window size the app gets set to.

Would you do this another way? Or did you mean use a global variable at the start and set it via the GraphicsHeight() then use the variable in the code?

And sorry to be a pain but could you explain the different logic to me for the else, elseif, endif vs the singular if then's. I'm just hazarding a guess but is it because my 'optional' version is considered as one statement and processed once per time through the loop while the singular 'if thens' allow for the potential of two if's activating the screen move?

Otus	(Posted 2009) [#6]

I'm not sure what you mean by too dependent. I was using GraphicsWidth() etc to make it less dependent on pre set constants and work for whatever screen/window size the app gets set to.

Would you do this another way? Or did you mean use a global variable at the start and set it via the GraphicsHeight() then use the variable in the code?

While using GraphicsWidth/Height makes it independent of screen resolution, it still always uses full screen. You could use a Global variable to make it possible to leave some screen space for UI. I prefer using the ViewPort, though, since that's pretty much what it's for.

And sorry to be a pain but could you explain the different logic to me for the else, elseif, endif vs the singular if then's. I'm just hazarding a guess but is it because my 'optional' version is considered as one statement and processed once per time through the loop while the singular 'if thens' allow for the potential of two if's activating the screen move?

Consider a situation where two keys are down. The ElseIf version only registers the first one, whereas the multi-If-version registers both.

jsp	(Posted 2009) [#7]

What Otus said!
Just imagine you need later a split screen, a canvas, or a window inside the window... no need to write another type with different settings. You could use a parameter in a create function to be fully flexible:

Type TStarField
	Field Width:int
	Field Height:int
	...

	Function Create:TStarField( Width:int, Height:int )
		Local StarField:TstarField = New TStarField
		StarField.Width = Width	'optional width-1 and save all -1 in the code... 
		StarField.Height = Height
		Return StarField
	End Function
...
End Type

Consider a situation where two keys are down. The ElseIf version only registers the first one, whereas the multi-If-version registers both.

Exactly, and that's what I meant with the diagonal move, where you need to press two buttons simultaneously. Your optional if statement would not allow that. Don't know if its important for your game though.

Juxta

(Posted 2009) [#8]

Thanks Otus and jsp that helps a lot!

No game at this stage jsp, just learning the ropes. Its been years since I've had to do any code and damn but I'm rusty. I've always learned better by doing but when I get around to my game I'll do a design doco first - hopefully so I don't make rookie mistakes like the ones you guys have helped me with. Once I get to that point I'll need to track down a competent artist (although I've been looking into programmer art and procedural generation and it looks interesting).

Thanks a lot everyone, you've all helped a lot... I've already made some changes and have added in a 'square' that I convert to a pixmap then move around on top of a scrolling 'starfield'. I'm hoping to add in some collision logic and background music today. The only issue is I get 'tearing' in windowed mode of the starfield, I figure its probably better to create the starfield, covert the whole thing to a tiled background, then scroll it... so more stuff to learn :)

Although first I think I might go back through the code and make some changes to my Types to make them a little more flexible based on jsp's feedback.

Grey Alien

(Posted 2009) [#9]

Quick glance, looks good to me, I'd understand it (I hope).

plash

(Posted 2009) [#10]

Tabs, instead of spaces, are best imo. They format the code in a good way without making removing them a pain (usually 4 spaces must be removed in-place of 1 tab) - not to mention tab's are one byte, whereas the space typically must be used four times to be the size of a tab.

Sledge

(Posted 2009) [#11]

'sf' should probably have a more meaningful name (something that reflects the fact that it is a collection) -- generally your variable and method names are really self-explanatory, so much so that the comments are mostly superfluous. R,G and B should probably be lower case for the sake of consistency... these are all picky little things because it's genuinely quite pleasant to read already.

One thing that irks me (sad?! who?! me?!) is resetting the stars to absolute positions when they move off-screen -- it means that, relative to where they should be, they fall slightly out of position. I would favour...
if x<0 x:+screenWidth

The only issue is I get 'tearing' in windowed mode of the starfield

Try changing the gfx driver -- on my machine, for example, OpenGL just never syncs whereas DirectX does. Neither in windowed mode, mind, but your mileage may vary.

I figure its probably better to create the starfield, covert the whole thing to a tiled background, then scroll it

Over-engineering is popular around here (dig dig!), but don't do this. On an actual tiled display you would either have to painstakingly work out a set of animated* tiles that fit together to make a full starfield (this would cost time for less freedom than you already enjoy) OR sacrifice a screen's worth of tiles and address them as if they were a screen sized bitmap (which would mean you'd be emulating a tiled display in order to emulate a bitmap display, which would be absurd!) Seriously, just plot/blit 'em!

*Because you're obviously going to animate this, right? With stars moving at different speeds to give the impression of depth, right? RIGHT?! :P

EDIT: Oh and I guess I'd use a list rather than an array, just because I find the resulting code a little cleaner.

Juxta

(Posted 2009) [#12]

Lots of great stuff there. Thanks for the help guys.

@Plash: Spaces are an old habit from my uni days when the professors would hammer into us how TABs rendering on different systems cannot be assumed to be consistent while spaces could, we'd lose marks on assignments if we used TABs for indenting.

Mind you now that I've tried it I've noticed that the MaxIDE keeps my indent level if I return after using them (it doesn't for spaces) so this is another huge plus for using TABs for indenting... now I just have to break those years of training.

@Sledge: Good call on the variable name for sf, spent so much time on all the others and that one slipped through :(

I like the relative positioning version for scrolling the stars you used. I'm updating as we speak.

And thanks for letting me know how problematic the tilemapping would be, I thought that it would save on processing if there was only 3 or so image objects to handle, but now I've stopped to think about it I realise how silly it is to try optimise this much.

Yes they are animated - basic at the moment (sinlge layer scroll with varying speeds) but I've created a larger 'star' version and want to play around with kind of a 'twinkling' effect as they scroll - so rather than keeping them as points to Plot I've converted them to Pixmaps and I'll do a variable speed 3 stage animation as they 'grow' and 'shrink' by a couple of pixels while keeping their relative positions. It'd be nice if I could apply opacity levels on a per pixel basis - but I've not seen an easy command to do that and I'm not sure how to do that on my own via code... yet! I'm thinking 3 layers at the end, with the back going slowest and only being 1 pixel while the top moves fastest and has the largest 'growth' animation. Then I get to add in the player ship :)

Once again thanks so much everyone you've all helped a great deal!

Back to the drawing board :)

TaskMaster

(Posted 2009) [#13]

The good thing about using tabs is that I can set my tab equal to 2 spaces in my IDE and you can set your tab equal to 4 spaces in your IDE. Then, if I view your code, the spacing is how I like to see it and when you view your code, the spacing is how you like to see it.

DavidDC

(Posted 2009) [#14]

I wouldn't make a loop variable a Type field. I understand you are trying to gain speed that way, but that kind of optimisation can lead to tangles down the track - eg calling one of your methods within another method's loop. I doubt you'd notice any speed difference using locals in the above.

I'd have some kind of capitalisation consistency happening - so you can easily distinguish between locals, fields and globals.

You might find ordering your functions/methods alphabetically handy.

Finally I tend to allocate one .bmx file per Type as a general rule. Makes for a lot of files but I find my code easier to navigate that way.

But this is just my personal preference. I'm sure there are plenty of others out there that'd disagree with one or more of the above.

Juxta

(Posted 2009) [#15]

Hi David,

Thanks for the feedback. I agree with you on the loop variable, my later versions have removed this (it was actually more like lazyness than concern for optimisation that saw me place it there - you know declare once then use anywhere - I've since slapped myself and I'm back to being more careful).

Yeah the capitalisation thing worries me - only because I'm used to coding in languages where case is important and with BlitzMax it's not. In general the method I'm used to (but still having trouble sticking to) is to use camel case, with lower case starts for all variables, all upper case for constants and starting with upper case for Types/classes. Not having had to deal with globals previously I'm uncertain as to whether I need to differentiate them from local or field variables - it would be nice if the BlitzMax IDE tracked variables in the code panel as well as the types and functions. I would also usually use lower case starts for function names - but the BlitzMax style capitalises all keywords and included functions so I've tried to do the same thing.

I do generally find ordering my functions/methods to be handy too - but I tend to group them more by function (eg: all the getter/setter calls together and all the draw functions together). But I generally try to keep my classes/types small so they are easily scanned through... which means I agree whole heartedly with your separate files for types. I'm a java coder from way back and I'm used to that methodology and it makes sense to me. I just wasn't sure it was a good idea with BlitzMax, so thank you for that.

johnnyfreak

(Posted 2009) [#16]

I would delete starcount variable. You can get array's lenght directly.

 myarray.length

For the use of Self, i tend to use it only when there's something like

  type foo
     field bar:int
     
      ...
     
     method setField(bar:int) ' same name for parameter and field!
          self.bar = bar
     endmethod
   
  endtype

Otherwise you can avoid using Self.

 Field R:Int, G:Int, B:Int 'colour variables

I tend to use uppercase names only for constants.
In general i think is useful using this conventions:

- variableNames in loer camel case: thisIsMyVariableName
- type name starting with T: TMytype
- constant in uppercase: MYCONSTANT

But these aren't rules, only my way of writing variables

Juxta

(Posted 2009) [#17]

Hi johnnyfreak,

Thanks for the feedback.

Thanks for letting me know about usage of self... it was throwing me a bit - I wasn't sure to use it explicitly in creation methods/functions (New and Create) or not. Part of the code you see is a bit of me experimenting to see what works and also to see what you guys think is the best practice. With new compilers its hard to know what it does and doesn't respond well to - or what potential issues I'm leaving myself open to by doing something one way over another.

I pretty much agree wholeheartedly with your variable naming convention - it's what I'm used to. Do you see there being a need to differentiate between local, global and field variables? I've got a tickle at the back of my head that says knowing a variable is global is important but I'm not sure whether to just add a g at the start of them instead or just leave it alone.

I'm not sure about removing the starcount variable, to my mind it serves a couple of purposes: for readability it gives explicit meaning to what is going on (if I just pass GraphicsWidth() to the array upon creation it loses some meaning), and also later on I'll expand upon the Type to have getter/setter variables to change startcount as well as catering for the potential instances where the actual amount of stars displayed is reduced but the same sized array is used.

johnnyfreak

(Posted 2009) [#18]

For global/Field:

- global variables in a type refer to the object
- field variables refer to the object instance
try this

SuperStrict

Type foo
	Field fee:Int = 4
     Global bar:Int = 4

     Method getBar:Int() 'this should be a function IMHO
	   Return bar
	EndMethod
	
	Method setBar(x:Int) 'this should be a function IMHO
	   bar = x
	EndMethod
	
	Method getFee:Int()
		Return fee
	EndMethod
	
	Method setFee(x:Int)
		fee = x
	EndMethod
EndType

Local a:foo = New foo
Local b:foo = New foo

Print "a_fee: " + a.fee + "; a_bar: " + a.bar
Print "b_fee: " + b.fee + "; b_bar: " + b.bar

b.setBar(10)
b.setFee(10)

'bar changed for a but i called only for b.
'fee is different
Print "a_fee: " + a.fee + "; a_bar: " + a.bar 
Print "b_fee: " + b.fee + "; b_bar: " + b.bar

For global variables in generale i don't have a fixed "rule"

Local variables in a function/method are variable that are deleted from the stack when the function call ends.

For starcount variable, it's the same. I like using length field for arrays because using a different variable for the same thing introduce a bit of redundancy. In C you don't know array size so you had to remember it with variables, bmax lets you know with length field or with Len() function.

Juxta

(Posted 2009) [#19]

Thank for that johhnyfreak,

That's interesting, even though you have instantiated 2 foo objects there is effectively only one 'bar' variable thanks to it being set global.

In terms of interest for myself - I'm assuming that the last one instantiated becomes the only version held in memory? But what is actually happening behind the scenes? Is it a case of one version being 'overwritten' or something else?

In some of the game programming books I've read (ages ago) for C/C++ they recommended using a lot of global variables and then using functions to change/access them directly rather than passing by value/reference to a function - due to impacts of pushing/removing from the stack... is this still considered good practice? Or has processor speed and available memory (especially with BlitzMax 2D programming) rendered the need for using this 'dangerous' practice unnecessary?

johnnyfreak

(Posted 2009) [#20]

sorry but i'm not so expert in developing games :( ( not yet! ), so i can't what are the "best practices".

Global variables are variable of the type
fields of the object.

So there's only one "bar" and one "fee" for each object