Wow! I'm famous.

After reading this entry, davw's feelings were obviously hurt. *shrug* He said something to the effect of "Yeah, write about it cruelly rather than telling me what to do instead."

First Lesson: If you're sensitive, quit. Now.

Second Lesson: There's an abundance of reference material on the vast majority of topics you will cover. If there isn't, it will be advanced enough that you will know that there isn't. Got that? Good.

Third Lesson: Good code is comprehensible. davw's MAPLOOPXY might be dependent on _xsize and _ysize. Or it might not. It appears to create two variables x and y. Or maybe it just fills them out? The positioning and number of subsequent braces suggests that it's a nested loop pair. Maybe it isn't; maybe it's a loop within a conditional - if there's a loop at all.

Assuming we have a loop or two, what kind? What are the conditionals? What's the minimum number of times this loop is guaranteed to run (think do .. while vs while vs for)? Are the loop counters concurrently updated, dependent on each other, completely independent? Are they reset by the macros, assuming they are global? Are previous values preserved?

I really could go on, and Washu and I did for a little bit. The point is, the MAPLOOPXY macro obscures meaning and intent, and as programming languages are even more of communicative notation between humans (even if the other human - the maintenance programmer - is yourself six months from now) than instruction for machines, it is important to preserve those properties.

So refactor, biatch.

Washu can be an ass, that's why we love him. If you haven't figured that out, you need to spend more time in #gamedev. [grin] Don't let it bug you. He knows what he's talking about, and he's like that with everyone. If you develop a little thick skin and pay attention to what he says, you'll learn a whole lot.

A time saver ? Time saver of what ? Certainly not debugging time. This has to be one of the worst "tricks" i've ever seen in my programming life..

Quote:A time saver ? Time saver of what ? Certainly not debugging time. This has to be one of the worst "tricks" i've ever seen in my programming life..

Unfortunately, traditional programming courses I've seen leave students with the impression that coding is what takes a lot of time - after all, their assignments/projects usually involve writing some throw-away code to solve one problem (i.e. small scope), which doesn't require much design, debugging or any maintainance. Testing is unheard of.

On their own, they deduce that saving coding time (a couple of keystrokes) is good. And then you start getting weird function/variable names (because obviously descriptive variable/function names waste time) and the ball keeps rolling.

I don't find it that bad when it comes from a student or someone who's in the beginning - just point him to the right direction.

EDIT:

Quote:A time saver ? Time saver of what ? Certainly not debugging time. This has to be one of the worst "tricks" i've ever seen in my programming life..

Unfortunately, traditional programming courses I've seen leave students with the impression that coding is what takes a lot of time - after all, their assignments/projects usually involve writing some throw-away code to solve one problem (i.e. small scope), which doesn't require much design, debugging or any maintainance. Testing is unheard of.

On their own, they deduce that saving coding time (a couple of keystrokes) is good. And then you start getting weird function/variable names (because obviously descriptive variable/function names waste time) and the ball keeps rolling.

I don't find it that bad when it comes from a student or someone who's in the beginning - just point him to the right direction.

EDIT: By the way, there are a few more bad practices in the code that deserve to be corrected:
- One should check whether the file was opened correctly or not
- If _mapdata is a raw array, it should be a vector. If it's a vector, it's better to use the 'at' member function instead of the [] operator, as it does bounds checking. If at profiling you find it's a bottleneck somewhere, use [] instead.

I don't know if this qualifies as bad practice, but closing 'saver' is done automatically by the destructor - so no need to do it manually.

That is one of the most god-awful techniques I have ever seen used... wow.. I seriously can't believe somebody would really do that.

For completeness,

prefer std::string to char*

and

using namespace std;

defeats the whole purpose of namespaces.

Quote:Quoting Oluseyi quoting DavW
Yeah, write about it cruelly rather than telling me what to do instead.

Well might I suggest something other than a macro? Scott Meyers, in his awesome book Effective C++ (now in it's 3rd edition! go go!), wrote that you can (almost) always replace a macro with a constant or a function - and you should.

In this case I would suggest taking a look at std::for_each to get some ideas.

You could change the macro into a function taking a functor that manipulates a map element:

template <class MAP_ELEM_MANIP>
void traverseMap( MAP_ELEM_MANIP manipulator )
{
  for(size_t x=0; x < _xsize; x++)
    for(size_t y=0; y < _ysize; y++)
      manipulator(_mapdata[_xsize*y+x]);
}

heh... I wrote that while checking the code, just noticed that _mapdata seems to be a linear array or maybe even a std::vector so then you could just use for_each directly ^_^ But that should be a usefull example anyway, I hope...

Well if you don't know what a functor is or how it looks like, I've written one (purely for demonstration) here on the right. I, the guy writing this :P, wouldn't go this far for this function. I'm gonna present a alternative way to save your elements in the end of this comment.

class saveMapElement
{
public:
  saveMapElement( std::ostream& out ) : saver(out) {}

  void operator () ( const MapElemType& element )
  {
    saver << element;
  }

private:
  std::ostream& saver;
}

Thus changing your original code to:

void C_Map::save(char *filename) {
	ofstream saver (filename,ios::binary);
	saver<<_xsize;
	saver<<_ysize;

        traverseMap( saveMapElement(saver) );
	saver.close();
}

Just a comment, the dtor of ofstream will close the ofstream object so this is kinda error prone. What if you forget about this line and start adding stuff like 'saver << myBanana' right below the close() call?

Alternative ending

Since you're using a linear storage of the map you can just use std::for_each directly on it, i.e. implementing traverseMap() like so:

std::for_each( _mapdata.begin(), _mapdata.end(), manipulator );

or

std::for_each( &_mapdata[0], &_mapdata[_xsize*_ysize], manipulator );

But! When dealing with streams you can use std::copy together with the stream_iterators. Thus:

std::copy( _mapdata.begin(), _mapdata.end(), std::ostream_iterator(saver) );

cheers