Better Coding Suggestion

Blitz3D Forums/Blitz3D Beginners Area/Better Coding Suggestion

Gauge

(Posted 2006) [#1]

The following is code from my text mud game. I was curious if there was a better way to write any of these?

note- axo means nothing just turn to 1 to term loop?? faster?

Function AddAlsohere(p.player)
	curr_id=0
	axo=0:xa=0
	While axo=0
	If area(p\curr_area)\ro[p\curr_room]\ro2[xa]=Null Then
	area(p\curr_area)\ro[p\curr_room]\ro2[xa]=New alsohere
	area(p\curr_area)\ro[p\curr_room]\ro2[xa]\alsoname$=Lower$(p\name$)
	area(p\curr_area)\ro[p\curr_room]\ro2[xa]\playerid=p\playerid
	p\alsoid=xa
	axo=1
	EndIf
	xa=xa+1
	Wend
	;writeLine p\stream, area(p\curr_area)\ro[p\curr_room]\ro2[p\alsoid]\alsoname$+" added ID: "+p\alsoid
End Function

Function DeleteAlsohere(p.player)
	xa=p\alsoid
	WriteLine p\stream, "Deleting ID: "+p\alsoid+" from "+area(p\curr_area)\ro[p\curr_room]\roomname$
	Delete area(p\curr_area)\ro[p\curr_room]\ro2[p\alsoid]
	xa=xa+1
	axo=0
	Print xa
	While axo=0
	If area(p\curr_area)\ro[p\curr_room]\ro2[xa]<>Null Then
	xa2=xa-1
	area(p\curr_area)\ro[p\curr_room]\ro2[xa2]=New alsohere
	area(p\curr_area)\ro[p\curr_room]\ro2[xa2]\playerid=area(p\curr_area)\ro[p\curr_room]\ro2[xa]\playerid
	area(p\curr_area)\ro[p\curr_room]\ro2[xa2]\alsoname$=area(p\curr_area)\ro[p\curr_room]\ro2[xa]\alsoname$
	playerx=area(p\curr_area)\ro[p\curr_room]\ro2[xa2]\playerid
	pl(playerx)\alsoid=xa2
	WriteLine pl(playerx)\stream, "Your also ID has been changed to "+p\alsoid
	Delete area(p\curr_area)\ro[p\curr_room]\ro2[xa]
	Else
	axo=1
	EndIf
	xa=xa+1
	Wend
End Function

jfk EO-11110

(Posted 2006) [#2]

looks ok to me, but (it's only a visual issue) you should use proper code formatting with nested stuff like: If, else, endif, While, wend.

octothorpe

(Posted 2006) [#3]

The only big issue is that you don't have any error checking for the case where you run out of elements in your array.

Style-wise: indentation, descriptive variable names. You'll probably get less feedback on code that isn't indented reasonably because most programmers won't like reading it. Good variable names are critical to readability. Change axo to "done" or "is_done" or "loop_done"; never name a variable a random collection of letters and numbers.

note- axo means nothing just turn to 1 to term loop?? faster?

That's micro-optimization. Don't even think about that. It's fine the way it is. Personally, I would make both loops infinite and Return from within them - but that's purely a style choice. Some people don't like functions which return from the middle. Whatever is simpler and more readable to you is what you should go with.

If you're seriously worried about efficiency, you could (a) reuse the freed element in DeleteAlsohere() instead of doing a linear search for a free element after having erased one and/or (b) replace your array with a linked list. But you shouldn't be worried until you can prove that these things are actually costing you performance.