• Advertisement
  • entries
    146
  • comments
    436
  • views
    198065

OH MY GOD

Sign in to follow this  

699 views

Alright, well, before we get started on doing some more work on our little access list project, I've got some code to discuss... something so horrible that it nearly...well, I'll just show it to you.

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

MAPLOOPXY
saver<<_mapdata[_xsize*y+x];
}
}
saver.close();
}


Now, if you don't see what is so horrible about this particular piece of code, please, stop reading my journal now, and leave. Now, the author of this particular piece of code gave the following excuse, when confronted with my shocked statement... " MAPLOOPXY is just a little timesaver" A what? A timesaver? I look at that code and I see a syntactic nightmare. You have a macro that creates two for loops (just a guess here) using two counters that are declared...somewhere. Not only that, but then this macro generated loop produces two temp variables x and y... talk about code that you can't maintain. How on earth would you know, if initially reading the code, that MAPLOOPXY use _ysize, _xsize or for that matter, produce the temps x and y. You wouldn't. Not only that, but you would notice that there are two entirely out of place closing braces... Jesus...talk about something to kill someone over. He then continued on saying: " i use the same look about 6 times in the same file for different functions". To which Oluseyi responded: " davw, then refactor or, as Washu would say, "Refactor, bitch!" Damn right, refactor. People wonder why many of us post against macro usage in C and C++ applications. Well, now you know why. Because people inevitably think to themselves: "Well gee if I do this it will be a time saver." But they don't actually consider the consequences of such actions, such as the ability to intuitively understand the code. So, in my own words, refactor bitch.
Anyways, next entry we'll continue on with our refactorings.
Sign in to follow this  


8 Comments


Recommended Comments

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.

Share this comment


Link to comment
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.

Share this comment


Link to comment
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..

Share this comment


Link to comment
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.

Share this comment


Link to comment
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.

Share this comment


Link to comment
For completeness,

prefer std::string to char*

and

using namespace std;

defeats the whole purpose of namespaces.

Share this comment


Link to comment
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

Share this comment


Link to comment

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

  • Advertisement