class Bitmap
{ bool BitmapLoaded;
public:
HBITMAP bitmap;
~Bitmap()
{
DeleteObject(bitmap);
}
void Load(LPSTR name)
{
bitmap = LoadBitmap(name);
BitmapLoaded = true;
}
Check()
{
if(BitmapLoaded == true)
{
if(!bitmap) PostQuitMessage(0);
}
}
};
Im so proud of myself!!
Author
Well i posted a thing about my code, and 2 people suggested how to make it better, and i made the class they suggested better!(well if there is a problem tell me) but they suggested to make a class for my bitmap, here is the code(the error checking is what i made up myself!)
Well the reason im so proud of myself, is that so far all ive been doing is understanding things, but now i just made my own new thing!!! (sorry for using "thing" all the time, but its really good to use when you dont want to explain everything!)
Good job! But I will add my own recommendations:
All just some suggestions! Other than that good work! [smile]
class Bitmap{ bool BitmapLoaded; HBITMAP bitmap; public: // ctors are good too ;) Clear the variables Bitmap() { BitmapLoaded = 0; bitmap = 0; } ~Bitmap() { // If they do not call Destroy manually Destroy(); } void Destroy() { // See if memory exists in this bitmap if( bitmap ) { BitmapLoaded = 0; DeleteObject(bitmap); bitmap = 0; } } bool Load( LPSTR name ) { // User tries to reload it if( bitmap ) Destroy(); bitmap = LoadBitmap(name); if( !bitmap ) return false; BitmapLoaded = true; return true; } void Check() { if(BitmapLoaded == true) { if(!bitmap) PostQuitMessage(0); } } }; All just some suggestions! Other than that good work! [smile]
Good job. 
Heres how you can make it even better:
-You need a constructor that sets BitmapLoaded to false. Otherwise it may be true before anything is open.
-In your Load function, you should check to make sure that LoadBitmap suceeded.
-In your destructor (~Bitmap) you may want to make sure a bitmap is open before calling DeleteObject on it.
-A Close function would be nice too.
edit:
I guess Drew_Benton just covered all of this. [wink]
Heres how you can make it even better:
-You need a constructor that sets BitmapLoaded to false. Otherwise it may be true before anything is open.
-In your Load function, you should check to make sure that LoadBitmap suceeded.
-In your destructor (~Bitmap) you may want to make sure a bitmap is open before calling DeleteObject on it.
-A Close function would be nice too.
edit:
I guess Drew_Benton just covered all of this. [wink]
Even more:
An even better way to do it would be to have an std::map using the LPSTR name as the key.
#include <vector>class Bitmap{ HBITMAP m_bitmap; LPSTR m_name; public: // ctors are good too ;) Clear the variables Bitmap(LPSTR name) { Load(name); } Bitmap() : m_bitmap(0) { /* nothing */ } ~Bitmap() { Destroy(); } void Destroy() { if( bitmap ) { DeleteObject(bitmap); m_bitmap = 0; } } bool Load( LPSTR name ) { // User tries to reload it if( m_bitmap ) Destroy(); m_bitmap = LoadBitmap(iname); if( !m_bitmap ) return false; m_name = name; return true; } void Check() { if(!bitmap) PostQuitMessage(0); } bool BitmapLoaded() { return bitmap != 0; } HBITMAP Image(){ return m_bitmap; } LPSTR Name(){ return m_name; }};std::vector<Bitmap*> Bitmaps;bool Load(LPSTR name){ Bitmaps.push_back(new Bitmap(name)); if(!Bitmaps.back()->BitmapLoaded()){ Bitmaps.pop_back(); return false; } return true;}void Destroy(){ while(!Bitmaps.empty()){ delete Bitmaps.back(); Bitmaps.pop_back(); } Bitmaps.clear();}bool Destroy(LPSTR name){ for(std::vector<Bitmap*>::iterator pos = Bitmaps.begin(); pos != Bitmaps.end(); pos++){ if((*pos)->Name() == name){ delete (*pos); Bitmaps.erase(pos); return true; } } return false;}An even better way to do it would be to have an std::map using the LPSTR name as the key.
Hmm, do you even need the bool BitmalLoaded? You could simply check if your HBITMAP Bitmap == 0 (you should set it to 0 in the constructor).
Author
Thanks!!, they are all good suggestions!, i thought people would make fun of me for being so happy about one thing, but all of you guys were nice!! yay, lol
I won't make fun of you, but I will say that the design isn't what it should be.
- Don't make separate "init" or "load" functions for objects, except as private helper functions shared between multiple ctors. Have the ctor do the actual work, and throw an exception if loading can't be done. (You must be sure to do any necessary cleanup before you throw the exception. In the case of the C APIs, usually there is nothing to do if you get a null pointer back, and you don't have your own stuff to clean up in the object, so we're all good.)
- Don't put a "is valid" flag into the object, as a result of the first point, because we now know any object that was actually created will be valid.
- Don't have public "check" methods that will verify an object is in a consistent state at runtime; the object is supposed to be responsible for keeping itself in a consistent state. For debugging purposes, make use of asserts instead, if you need to.
- Don't have public data if you can avoid it; that makes it harder for an object to maintain its integrity.
- Do give your objects "useful things"; in this case a name is useful. You will want to expose a "public interface" that includes things your Bitmap object can do, rather than what it "has". In the real world, asking people nicely to do things works better than taking their stuff, playing around with it and handing it back - so it is with OO too. I don't know what you want to "do" with your bitmaps, so I won't fill any of these functions in. But as an example, you might have a "void draw(ThingABitmapCanBeDrawnUpon x)", where the actual type of course depends on what graphics API you're using :)
- Do use std::string. (HEY ROB, YOU DO KNOW WHAT "LPSTR" IS A TYPEDEF FOR, YES? [grin])
- Do use constructors, and initializer lists.
- Do heed the "Rule of Three" whenever your object "owns" a pointer to some resource: if you require any of (destructor, assignment operator, copy constructor), you generally require all.
Putting that together:
Nice and simple (once you understand it and take out all the 'tutorial' comments). :)
- Don't make separate "init" or "load" functions for objects, except as private helper functions shared between multiple ctors. Have the ctor do the actual work, and throw an exception if loading can't be done. (You must be sure to do any necessary cleanup before you throw the exception. In the case of the C APIs, usually there is nothing to do if you get a null pointer back, and you don't have your own stuff to clean up in the object, so we're all good.)
- Don't put a "is valid" flag into the object, as a result of the first point, because we now know any object that was actually created will be valid.
- Don't have public "check" methods that will verify an object is in a consistent state at runtime; the object is supposed to be responsible for keeping itself in a consistent state. For debugging purposes, make use of asserts instead, if you need to.
- Don't have public data if you can avoid it; that makes it harder for an object to maintain its integrity.
- Do give your objects "useful things"; in this case a name is useful. You will want to expose a "public interface" that includes things your Bitmap object can do, rather than what it "has". In the real world, asking people nicely to do things works better than taking their stuff, playing around with it and handing it back - so it is with OO too. I don't know what you want to "do" with your bitmaps, so I won't fill any of these functions in. But as an example, you might have a "void draw(ThingABitmapCanBeDrawnUpon x)", where the actual type of course depends on what graphics API you're using :)
- Do use std::string. (HEY ROB, YOU DO KNOW WHAT "LPSTR" IS A TYPEDEF FOR, YES? [grin])
- Do use constructors, and initializer lists.
- Do heed the "Rule of Three" whenever your object "owns" a pointer to some resource: if you require any of (destructor, assignment operator, copy constructor), you generally require all.
Putting that together:
// Represents a named picture, which can possibly draw itself and do other nice// stuff, without letting the calling code mess up the internals too badly.class Bitmap { std::string name; HBITMAP bitmap; public: Bitmap(std::string name) : name(name), bitmap(LoadBitmap(name.c_str())) { // In the initializer list, we set our name and bitmap pointer // appropriately. If the pointer is null, we have a bad object and should // throw an exception: if (!bitmap) throw std::exception(("file" + name + " not found").c_str()); // The concatenation works because the middle item is a std::string. // If we throw the exception, then the 'name' destructor is called and there // is nothing to do with the 'bitmap' pointer, so we should be // exception-safe. } ~Bitmap() { DeleteObject(bitmap); } // We'll do a copy constructor, and then implement assignment in terms of // that using the very nice "copy-and-swap idiom". // To make a copy, we'll copy the name in the obvious way (relying on the // std::string copy constructor - if we were using char*'s we'd have to do // more work), and "copy" the bitmap by reloading it from file. There is // probably a better way to do that but I don't know what it is because I // don't know the Windows API :) Bitmap(const Bitmap& other) : name(other.name), bitmap(LoadBitmap(other.name.c_str())) { // We'll throw the same exception here, just in case the file got deleted // in the middle of the program run or something o_O if (!bitmap) throw std::exception(("file" + name + " not found").c_str()); // I don't like this duplication really, but it's only one repeated line and // there isn't really much that can be done about it... } // And now, here's an assignment operator. Note that the copy-and-swap idiom // preserves exception safety: if the copy-construction fails, then the // exception skips past the swap, so there is no change to the current object. // It also will not fail on self-assignment, and is just nice in general :) Bitmap& operator=(const Bitmap& other) { Bitmap temp(other); std::swap(*this, temp); // return self to allow for operator chaining. return *this; }}; Nice and simple (once you understand it and take out all the 'tutorial' comments). :)
I'd just like to show how this statement has some redundancy (don't worry it's common amoung beginners):
if(BitmapLoaded == true)
An if-statement always evaluates the expression in the brackets and then decides whether to execute the following code block, or instead the else code block if one exists, right.
That said, the expression within the brackets is a boolean expresion. == takes two operands of the same type and produces a boolean answer of true or false. e.g. if (x == 3)
However BitmapLoaded is already a boolean and in this case the expression can be simplified if you look at the following table...
A | A == true------+----------false | false true | true
So as you can see you can simplify that expression to just:if(BitmapLoaded)
This is very nice especially if you name your variables like this:
bool gane_is_paused;if (game_is_paused) // do something...
Basically == true is ALWAYS redundant.
Author
Hmmmm, i kinda wrote that pretty fast, ussually i do just put if(stuffhappens)or
if (!stuffhappens), so, yay, and i could someone tell me what std is for? or means, i know its other meaning, lol, but what is its programing meaning? because the tutorial i learned it from, didnt have that in it.
if (!stuffhappens), so, yay, and i could someone tell me what std is for? or means, i know its other meaning, lol, but what is its programing meaning? because the tutorial i learned it from, didnt have that in it.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
