How to wean from the Goto / Gosub style programming?

[zxtunes.com]

My friend programs on PureBasic of nearly 7 years.

You could see its game: http://www.purebasic.fr/english/viewtop ... 14&t=54133

[source code sample: https://dl.dropbox.com/u/6439155/Otter%26Fench.pb]

However its level and style of a code doesn't develop absolutely.

It as well as uses 7 years ago GOTO and GUSUB.

Loads pictures separately, doesn't use a constant, variables calls short names, doesn't use structure, etc.

As a result the code 3-4 times bigger turns out than had to be, and anybody except the author can't understand it.

Couple of words about the person:

It programmed 6 years on the Z80 assembler for ZX Spectrum (did games). And as wrote 8 bit music.

Now it has a musical group. as it issues the magazine and writes books.

From everything the person of a humanitarian is higher listed clearly that.

It is all about it? Or matter in traits of character? (sees things as a whole, but isn't able to concentrate on trifles)

[MachineCode]

anybody except the author can't understand it

It's very much spaghetti code, but it can be followed easily. Well, I can anyway, coming from a C64 background.

At the end of the day, the code does the job, and if it's ugly, so what? When you watch a good movie, you don't care that it took 50 or more takes to shoot one scene: all you see is a good scene. Code is the same: the final result is all that matters.

Having said that, people argue that it should be easier for other people to read. Sure, if the intent was always to share the code. Until this thread, nobody else here was involved in this game, so the author had only himself to consider, and thus could code how he wanted. There's nothing wrong with that. There's even an old joke that applies: "It was hard to write, so it should be hard to read."

People say avoid Gosub/Return, but they're really just another variation of Procedure/EndProcedure anyway, in that you're just jumping somewhere else in the source and then coming back. You can use either method if you code defensively about it, but Procedures are easier for reasons I won't go into now.

Speed-wise, if the code executes slowly, it can probably be optimized more. Look at this slow block:

Code: Select all

If weapon=900:wanim=6:EndIf
If weapon=901:wanim=4:EndIf
If weapon=902:wanim=15:EndIf
If weapon=903:wanim=10:EndIf
If weapon=904:wanim=10:EndIf
If weapon=905:wanim=12:EndIf
If weapon=906:wanim=12:EndIf
If weapon=907:wanim=12:EndIf
If weapon=908:wanim=12:EndIf
If weapon=909:wanim=10:EndIf
If weapon=914:wanim=15:EndIf
If weapon=910:wanim=0:EndIf
If weapon=912:wanim=0:EndIf

The "weapon" variable is checked a massive 13 times, when it only needs to be checked ONCE! If weapon=900, then it' still going to be checked a further 12 times (in the following lines) for no reason. This slows down the app/game, and if lots of this sort of code exists, then the app may slow down dramatically. The above block should be recoded like this:

Code: Select all

Select weapon
  Case 900 : wanim=6
  Case 901 : wanim=4
  Case 902, 914 : wanim=15
  Case 903, 904, 909 : wanim=10
  Case 905 To 908 : wanim=12
  Case 910, 912 : wanim=0
EndSelect

This is smaller, cleaner, easier to read, and will execute faster because "weapon" isn't checked 13 times. Also, constants should be used for the numbers, to make the code even easier to follow. Here's a modified example that makes it easy to see which weapon was selected (instead of trying to remember what weapon "900" is):

Code: Select all

Enumeration
  #pistol
  #machinegun
  #shotgun
  #supershotgun
EndEnumeration

Select weapon
  Case #pistol : wanim=6
  Case #machinegun : wanim=4
  Case #shotgun, #supershotgun : wanim=15
EndSelect

At the end of the source, your friend is also creating variables that are only used ONCE (bad!) and repeating strings TWICE when a variable should be made for them. Also, RGB is used many times for the SAME color, so a single variable should be used for it instead. Look:

Code: Select all

b=TextWidth("Resolution")
DrawText(xraz/2-b/2, 180 , "Resolution", RGB((255),(255),(255)) )
b=TextWidth("Esc - Exit")
DrawText(xraz/2-b/2, 240 , "Esc - Exit", RGB((255),(255),(255)) )
b=TextWidth("M - Shop")
DrawText(xraz/2-b/2, 260 , "M - Shop", RGB((255),(255),(255)) )

Optimised to reduce repetition and make easier to read:

Code: Select all

col=RGB(255,255,255)
t$="Resolution" : DrawText(xraz/2-TextWidth(t$)/2, 180 , t$, col)
t$="Esc - Exit" : DrawText(xraz/2-TextWidth(t$)/2, 240 , t$, col)
t$="M - Shop" : DrawText(xraz/2-TextWidth(t$)/2, 260 , t$, col)

Pass these tips onto your friend. They're only a starting point, but he'll benefit from them.

[luis]

@zxtunes

Probably coming from z80 assembly he likes thinking the same way when programming with PB. Gotos and returns. If he's not interested in learning different tools of the trade there is nothing you can do about it.
If he's curios, just show him the advantages of structures and constants and he will certainly integrates at least them in his code.
And the poetry of loading something in a loop instead of putting 3124 loadsprites in his code. And Datasections.
I'm quite sure these are all things he could appreciate and relate to easily.
All the rest is debatable.

The "weapon" variable is checked a massive 13 times, when it only needs to be checked ONCE!

Unfortunately, that is just not possible. The advantage of select/case in this case is it jumps out when a match is found. And it will be noteworthy only when opposed to an insanely long if/then sequence.
What I want to stress is: reading the high level code it may seems weapon is checked only at the top. But that's not the truth. When you make comparisons you need to think how the high level code is translated. Don't speculate the high level code is what is being executed.

Not to diminishing your suggestions in general, they are valid. Probably they would not cause a tangible impact in this case.

[MachineCode]

reading the high level code it may seems weapon is checked only at the top.

I never said it's checked only at the top. I said it's checked 13 times in that block, one line after the other. The value of "weapon" doesn't change at every line, so it doesn't always need to be checked 13 times. That's my point. (I shouldn't have said it only needs to be *checked* once; that was a mistake. I should've said *matched* once).

[luis]

I considered that option, if you didn't think it was checked only at the top, perfect.
I can apologize if you like.

But you said "it needs to be checked only once", and my comment was really about that. It's not possible, with all the considerations that follow.
To the benefit of someone else reading this, if you like.

[MachineCode]

But you said "it needs to be checked only once", and my comment was really about that.

Yes, that was the wrong choice of word. I should've said "matched" once. No need to apologize; it was my error.

[luis]

[Demivec]

It is all about it? Or matter in traits of character? (sees things as a whole, but isn't able to concentrate on trifles)

@zxtuntes.com: There is more than one reason that is possible. Some have to do with limitations, others with choices.

If you were to compare the use of a programming language to painting you could say that your friend learned how to 'paint' well enough in Z80 assembly to produce games. PureBasic adds many more colors and several more brushes that can be used. As MachineCode pointed out, it isn't necessary to use every color or every brush if nobody is ever going to see the painter at work, people will judge what is painted and not so much how it was painted. Even with the additional things that PureBasic offers the real secret is practice. Your friend has to practice doing things differently and learn to take advantage of the things PureBasic offers that Z80 assembly didn't.

A major reason to improve on programming skills is that things can be done faster with the right tool and the results will better match what the programmer intended when they take advantage of the larger palette available.

The use of longer variable names and the like will make things easier to read later when your friend's memory has faded.


@MachineCode, luis: Regarding the programming puzzle of checking values, it can be done with a lookup. Wouldn't that qualify as needing to check only one value?

Code: Select all

Dim weapons(14) ;initialized to the values {6, 4, 15, 10, 10, 12, 12, 12, 12, 10, 0, -1, 0, -1, 15};  -1 is for unused index values.
wanim = weapons(weapon - 900)

[luis]

@demivec

yes, I was talking about doing that using the conditional statements of the language (if/then, select, etc.), not algorithmically building something ad hoc but you are right in pointing that out, thanks
It is not unsolvable, you can even use maps at this point
Frankly it's all overkill and I can understand some simple cascading IF in this case.