my level class

[Clayton]

hey guys I'm making a pacman type game right now and i have a level class, and i was wanting to know if i could be doing this any better ive attached it along with a file with a level in it, plz comment on how it could be improved

[Cervantes]

You shouldn't hardcode the file name. Rather, make that "getInfo" procedure called "load (filename : string)". So now you can write,

code:

level -> load ("map1.txt")

That's quite readable. Good.

Indentation. Why didn't you indent the code in the procedures?

Your levelInfo flexy array should start at 1 .. 0. Then in your getInfo (or load) procedure, you move the 'new ...' line up one. This means that if your file contains no data, your levelInfo array will be empty, as it should be.

The create procedure should have some commenting. It's not obvious what it is doing. It seems to be drawing something, which is why I question why it's called "create". clr1 and clr2 could be better named: wall_colour and space_colour, for example.

[Clayton]

all right thanks for the tips i was actually thinking of doing something along the lines of your filename idea, except the filename is just a number concatenated on to another filename in the class, to allow for easier identification of files (multiple levels etc.) um its called create because its creating the level but the coloring names are a good idea, i just named them that because i was drawing a blank on what to call them thnx again for the tips

[Clayton]

ok i have done some work to it and this is what ive come up with (using the suggestions provided by Cervantes )

Turing:

class Level     export load, create     var levelInfo : flexible array 1 .. 0 of string     proc load (levelNumber : string)         var stream : int         open : stream, "level" + levelNumber + ".txt", get         loop             exit when eof (stream)             new levelInfo, upper (levelInfo) + 1             get : stream, levelInfo (upper (levelInfo))         end loop         close : stream     end load     proc create (wall_color, space_color, size_ : int)         var x, y : int         const size : int := size_         y := maxy - size         for j : 1 .. upper (levelInfo)             x := 0             for i : 1 .. length (levelInfo (j))                %draws the level depending on what symbol was taken from the file                 if levelInfo (j) (i) = "#" then                     Draw.FillBox (x, y, x + size, y + size, wall_color)                 elsif levelInfo (j) (i) = "." then                     Draw.FillBox (x, y, x + size, y + size, space_color)                 end if                 %moves over one square to continue drawing the level                 x += size             end for             %moves down to continue drawing the level             y -= size         end for     end create end Level View.Set ("graphics:500;500") var level : flexible array 1 .. 1 of ^Level new Level, level (1) level (1) -> load (intstr (upper (level))) level (1) -> create (brightred, brightgreen, 40)

let me know what you think of it

[Cervantes]

Better. I still have some issues, though. I'm just a picky guy.

The load procedure: If you're going to only pass in the level number, why pass it in as a string? Passing it in as an integer and converting it within the proc makes more sense to the outside world. After all, the levels are integers.

Actually, they're natural numbers. You might want to try using natural numbers (nat) instead. Also, outputting some sort of error if the file doesn't exist would be good (File.Exist).

I'm still not seeing why you called it 'craete'. To me, that looks like it would be a draw procedure. A create procedure would, in my mind, randomly create a level. The creation process was done by you in a text editor when you made that level1.txt file.

And the final point. In the 'create' procedure, why did you declare a constant 'size', when you could have just used the parameter value, 'size_'? (And after getting rid of that constant, you could name that parameter 'size' so it's nicer).

[Clayton]

i can see where your coming from for the create procedure (finally), and ya i guess it would make more sense to pass natural numbers in the parameters, i just thought i would do that to make the concatenation nicer to read I wasnt too worried about missing files because i was just going to use this for myself but i can see where your coming from on that note as for the constant 'size'

[Gandalf] wrote:

I uh.. have no excuse

[Cervantes]

SuperFreak82 wrote:

ya i guess it would make more sense to pass natural numbers in the parameters, i just thought i would do that to make the concatenation nicer to read

One of the great things about OOP is it compartmentalizes your code.

This means that someone else can use your chunks of code -- your classes -- without actually understanding how they work, internally.

But people can only use your classes effectively if they can figure out how they work, externally. They need to understand what the methods do and what arguments they should be given.

The easiest way to achieve this is to follow The Principle of Least Surprise. Things should do what they are expected to do, with as few surprises as possible. Thus, if a level is designated by a natural number (level 1, level 2 and so on) then the code to load a level should take a natural number as its argument.

[Clayton]

yep i understand that now thnx for all the help on this, hopefully ill get this together soon and be able to do stuff with it um and for the quote above:

Cervantes not [Gandalf] wrote:

I uh.. have no excuse

[Delos]

SuperFreak82 wrote:

I wasnt too worried about missing files because i was just going to use this for myself...

In any case, some sort of Error catching is really quite useful. Turing's built in Error. module isn't bad at all - I personally am a huge fan of Error.Halt(). For instance:

code:

proc load_file (name : string)    if not File.Exists (name) then       Error.Halt (name + " does not exist!")    end if end load_file

This sort of thing can be useful even for post-compilied debugging. Once the entire proggie is made, identify places of possible error and then create an error sheet for them. You could then echo things like:

code:

Error.Halt ("Error 213:  File " + name " not found.")

Not only would that look really impressive and professional, but later when people are reporting errors to you, you have a reference to work from.

[Clayton]

ok i like that idea so another way to use this could be like this?

Turing:

proc create (wall_color, space_color, size_ : int)         var x, y : int         const size : int := size_         y := maxy - size         for j : 1 .. upper (levelInfo)             x := 0             for i : 1 .. length (levelInfo (j))                %draws the level depending on what symbol was taken from the file                 if levelInfo (j) (i) = "#" then                     Draw.FillBox (x, y, x + size, y + size, wall_color)                 elsif levelInfo (j) (i) = "." then                     Draw.FillBox (x, y, x + size, y + size, space_color)                 else                     Error.Halt ("Corrupt or Tampered File")                 end if                 %moves over one square to continue drawing the level                 x += size             end for             %moves down to continue drawing the level             y -= size         end for     end create

also are there any fcns that i could/should add to this class? (as i said this is going to be a pacman game)

[Cervantes]

I think you should leave it as

code:

if levelInfo (j) (i) = "#" then     %wall else     %empty space end if

You might want to put something in the map file like the starting positions of the enemies. We know they start in an empty square, so that's okay. But we can't represent them as a ".". They'd be represented as "1", "2", "3"... for the first, second, third... enemy.

The level's data will be needed by other parts of your program, particularly for determining which move is legal (no moving into walls). So you'd probably want to export `levelInfo', huh? Here's another problem with Turing: it won't let you export a flexible array from a class.

Why? Whatever the reason is, it's the same reason that you can't return a flexible array from a function, because that's essentially what you're doing when you export a variable: a public function is automatically created that has the same name as the variable and simply returns the variable's value. So since we can't return a flexible array from a function, we can't export it from the class.

How do you deal with this? One possibility is to put data in your level file specifying the width and height of the map. Use this information to create an array, but not a flexible one this time. Note that this means you'll have to move the array declaration inside your `load' procedure.

Another possibility: is every level of Pacman the same size? If so, you could just make your array static, right where it is.

There's more ways to get around this, I'm sure. If you don't like either of these, you might discover some new ones yourself.

[Clayton]

unfortunately each level in the game is not the same size, much like the actual arcade version. some of the levels are 16 squares high and some are around 25-30 so... ya, a static array wont work, but something i just thought of is to create my flexy array, get all the info i need, then create a static array using the bounds of my flexy array, and export the static one, if your not thrilled with this idea let me know plz

[Cervantes]

SuperFreak82 wrote:

but something i just thought of is to create my flexy array, get all the info i need, then create a static array using the bounds of my flexy array, and export the static one, if your not thrilled with this idea let me know plz

That's essentially the same as inserting in each file the size of the map and using that to create a static array. Except with this idea of yours, you're making another array and then not using it, which is rather a waste of memory space.

If you don't like the idea of inserting the size of the map in each file, you could do it this way: read the first line in to determine the width of the map. Then use the size of the file, in bytes, divided by the width of the map to determine the height of the map. Make your static array from this information.

To get the size of the file, you have to use File.Status. A procedure. *sigh*

[Clayton]

I think ill just input the bounds of the array into the file to simplify matters, all i need is the upper bounds of the array, i can derive the length using the length function, thnx for the help Cervantes