Sign in to follow this  
dustydoodoo

Im so proud of myself!!

Recommended Posts

dustydoodoo    100
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!)
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);
               }
           }         
               
}; 
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!)

Share this post


Link to post
Share on other sites
Drew_Benton    1861
Good job! But I will add my own recommendations:


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]

Share this post


Link to post
Share on other sites
Rayno    511
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]

Share this post


Link to post
Share on other sites
Rob Loach    1504
Even more:

#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.

Share this post


Link to post
Share on other sites
dustydoodoo    100
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

Share this post


Link to post
Share on other sites
Zahlman    1682
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:


// 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). :)

Share this post


Link to post
Share on other sites
iMalc    2466
I'd just like to show how this statement has some redundancy (don't worry it's common amoung beginners):[code]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.

Share this post


Link to post
Share on other sites
dustydoodoo    100
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.

Share this post


Link to post
Share on other sites
Zahlman    1682
Quote:
Original post by dustydoodoo
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.


std is short for "standard", and it's the name of a namespace. Namespaces are a tool that C++ provides for organizing definitions (and globals) so that you can avoid naming collisions while still not having to do weird "name mangling" all the time.

The usual example that is used deals with std::vector: in game programming, or anything graphical for that matter, it is common to create types with two or three numeric values and call them "vectors" - which they are, in the mathematical sense. Were it not for the namespacing, you'd have a problem if you wanted to use the library 'vector' container and your own 'vector' class (for the mathematical entity) in the same project: basically, you'd have to rename one of them (yours, since you don't get to pick names for stuff in the standard library).

In the C days, this 'renaming' would usually consist of putting some prefix on each identifier, which is annoying and somewhat error prone - you have to remember which one you want. The namespace concept gives you the flexibility to avoid typing the "prefixes" all over the place (via a using statement), but still allows you to make your intention clear when there would be a conflict:


{
using namespace std;
// everything in this block that could be interpreted as something in the std
// namespace, is.
}

{
using std::vector;
// inside this block "vector" means std::vector, but e.g. "string" means your
// own string class, not std::string.
}

{
// inside this block, "vector" means your struct that represents mathematical
// vectors, not std::vector. If you need to use a std::vector, you can still
// get it by typing out std::vector in full.
{
using namespace std;
// Of course, scopes nest, and 'using' declarations follow the same scoping
// rules as local variables: thus in the inner block, we are using namespace
// std, but after the close brace...
}
// here, we no longer use namespace std.
}



One final note: Avoid using declarations, and especially do not use full namespaces, in your header files. This is because you will probably be putting them at file scope, thus they will basically affect everything in every file that includes them... a debugging nightmare when a name collision happens.

Share this post


Link to post
Share on other sites

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

Sign in to follow this