raocow's talkhaus

WhattayaBrian wrote:

Ashan wrote:BINDING OF ISAAC STUFF

Code critique:

addToInventory(), heldTrinket(), and addToConsumables() should all take const refs instead of value arguments.

getPennies(), printInventory(), printTrinket(), and printConsumables() should all be const member functions.

heldTricket() is a bad function name. It sounds like an accessor for getting your currently held trinket, rather than a function that changes it.

Be careful about using C arrays. For one, it's very rare that it's acceptable to have your size be a magic number. Should generally have something like "const unsigned MAX_CONSUMABLES = 50;". The second thing is that you're not bounds checking in addToConsumables(). The aforementioned variable would help with that.

However, really, just use a vector instead.

Your exit logic is a bit convoluted. It's not terrible, but it's somewhat strange how you're setting flags vs doing things directly. For example, "itemFlag = true;" could just be replaced with "break;" and the program wouldn't change. It could be cleaned up a bit to make it easier to understand.

In consumables(), items(), and trinkets(), you're using a switch case for a bunch of sequential numbers. In situations like this, you can pretty easily convert it into using an array:

Code: Select all

std::string trinkets()
{
	static const unsigned NUM_TRINKETS = 5;
	static const char * trinkets[MAX_TRINKETS] = { "Ace of Spades", "Counterfeit Penny", "Fish Head", "Liberty Cap", "A Missing Page" };
	int x = rand()%5;

	if(x >= 0 && x < NUM_TRINKETS)
	{
		return trinkets[x];
	}
	else
	{
		return "error";
	}
}

I didn't compile this, so I hope it works! A strategy like this makes it a lot easier to add new items in the future.

WhattayaBrian wrote:

Code: Select all

std::string trinkets()
{
	static const unsigned NUM_TRINKETS = 5;
	static const char * trinkets[MAX_TRINKETS] = { "Ace of Spades", "Counterfeit Penny", "Fish Head", "Liberty Cap", "A Missing Page" };
	int x = rand()%5;

	if(x >= 0 && x < NUM_TRINKETS)
	{
		return trinkets[x];
	}
	else
	{
		return "error";
	}
}

RenaBeach wrote:since you have control over the value of x there's no reason to do error checking (it just makes your code look flimsy to be honest)

WhattayaBrian wrote:

RenaBeach wrote:e: except i saw that you were already reading it

You could set it up as a function that takes a list and returns a random value out of it, which you'd only need one of, and then you'd call it by passing in one of your three lists.

but type safetyyyyyyyy (but i guess they're already strings, lol)

RenaBeach wrote:out of curiosity, what do you feel about C++ exceptions?

I'm wondering if I should start using them but I've never really been convinced with them

typedef system_clock high_resolution_clock;

RenaBeach wrote:constness looks okay (except for trinkets()/items()/consumables couldnt they be const too?)

std::string items const(){
...

std::string items; const(){

RenaBeach wrote:also now would be a good time to learn to organise your code into headers and source files (and probably stop inlining functions unless they're simple accessors or mutators)

...you should not depend on the inline specifier to cause a function to be inlined.