Cracktro :)

The Partridge Family were neither partridges nor a family. Discuss.
Byteraver
Posts: 60
Joined: May 19th, 2016, 6:48 am
Location: Belgium, Europe

Re: Cracktro :)

Post by Byteraver » October 27th, 2019, 11:30 am

Hi albinopapa!
Thanks again for your considerable effort in trying to get me on track, it is most helpful and appreciated. You are a good teacher :) Sorry I write and ask so much, but I just enjoy discussing these topics so much :) :)
About your other remarks:
Prefer using the STL algorithms over raw loops when possible.
I discovered their existence by accident I believe, and have no experience with them really. Something I must dive into I guess. One of many ;)
Prefer range based for loops ...
They look nice, however I often find myself halfway changing them back to range based loops because I need the index anyway. I shall try to use them where ever I can.
Prefer index based loops over pointer or even iterator arithmetic...
I thought pointer arithmetic would be faster, but maybe I underestimate the compiler. I remember a talk about optimization where the speaker went something like "Don't try to help the compiler!". So maybe I should stop doing that.
Prefer more descriptive names for variables...
I read somewhere that variable names should be shorter when less important (more local, less global) and longer if otherwise. Maybe I took this one too far ;)
Space out your code every few lines...
You are right, it is just that I like to get as much code on the screen as possible so I don't have to scroll all the time. Maybe I need a bigger monitor ;) Jokes aside, what really needs to be done is breaking functions up into smaller ones.
The UpdateModel() function should be used to update all your objects, and the ComposeFrame() function is used to draw them...
Aha, I was wondering why there were two of'm :D Thanks for clearing that up
Const functions
You are right, however, when developing, I found that sometimes I start to declare functions in a class as const, only to discover at the very end it is not going to be possible, and I have to undo all of it. It is the kind of thing I postpone till last and then forget about. But shouldn't :)
Prefer using constructors over Init() functions
I know, right? When you see an init() function, it usually means I couldn't find a way to properly initialize it in the constructor. Doesn't mean it can't be done, only that I didn't see how.
local variables can quite often be declared const
I started doing that more recently. I shall try to do it more systematically
Prefer std::unique_ptr over new/delete...
I don't understand safe pointers yet. The syntax looks quit intimidating. Another thing to dive into...
Prefer std::string over const char* in most cases
Another thing I started doing more lately. Initially, I wanted to only use my own code, no STL, to keep full control over the codebase. But I changed my mind, as I will never be able to make a class as good as the STL, and the STL is part of the C++ standard anyway.
Use move semantics ...
Another advanced topic I'm a bit intimidated by. I feel like I could try to use it, but would have no way to test it, causing some unfixable bug two months later.
It would probably be better to just pass a Font const& to functions needing the font
Maybe there was a reason to use a pointer, I don't remember. I'll have to look into it.

About the code duplication between sprite & Graphics classes: this has bothered me indeed, but I had yet to come up with a decent solution. One would be to always use the Sprite class functions, and create a special class that is actually the video memory instead of some sprite floating around in heap memory. While I think I kind-of understand your template code example, I could never come up with such a solution, it is way too advanced for my current level. Thanks for the suggestion though! Maybe I can try to implement it.
prefer constexpr over macros...
I don't think I use macro's, since I don't know how these work ;) Same goes for constexpr(). I use only basic "#define" statements for constants. Should I avoid these entirely?
Functions in the standard library that begin with an underscore should never be used
Oh man, you nailed it there. When I installed VS2019 yesterday it gave an error on exactly that function, and thanks to your "preemptive strike" I could fix it right away. You saved me there!! For which I am very grateful indeed.

When you corrected my notes you wrote:
- As arguments to a function, pass the underlying object by reference or const reference
- As arguments to a functor or function pointer supplied to the standard algorithms,
- pass unique_ptr by reference or const reference
- If you are transferring ownership, then transfer by value
Did you mean to say this?
- As arguments to a function, pass the underlying object by reference or const reference
- As arguments to a functor or function pointer supplied to the standard algorithms, pass unique_ptr by reference or const reference
- If you are transferring ownership, then transfer by value
Consider using the C++ random library instead
All right, should be an easy fix (I hope)
Consider using the C++ chrono library
I believe Chili made a video about it. Using the framerate counter as clock (as I did so far) is questionable to say the least, indeed you are right. Another subject I must delve into :)

About the XMLParser:
Your criticism about the poor choice of variable names is absolutely fair, not petty ;) I would have used the erase function if I knew it existed :D . I'll update the code, thanks for the suggestion.

Thanks for explaining the use of the auto keyword, I understand now why you did it this way. While I knew about auto, I shall refrain from using it for the time being, as I want to force myself to "properly" declare variables so I understand what I'm doing. When I have more experience, I can switch to auto. The appeal is obvious, but I feel I have not earned the right to use them yet, if that makes sense.

Thanks for re-explaining the in/out function parameter thing. I'm sorry I made you repeat yourself, but coming back to a subject and slightly rephrasing it did help me understand it better.

Byteraver
Posts: 60
Joined: May 19th, 2016, 6:48 am
Location: Belgium, Europe

Re: Cracktro :)

Post by Byteraver » October 27th, 2019, 2:52 pm

So I changed the code of the trimTrailingSpaces and created a function:

Code: Select all

std::string& XMLFile::trimTrailingSpaces( std::string& trimStr )
{
	for (;;) {
		size_t strLen = trimStr.length();
		if ( strLen == 0 )
			break;
		strLen --;
		if ( trimStr[strLen ] != ' ' )
			break;
		trimStr.erase( strLen );
	}
	return trimStr;
}
and later on called it this way:

Code: Select all

endMarker = trimTrailingSpaces( endMarker );
Is this what you had in mind? Would this invoke the c++ move thing? Because I tried to make it work otherwise (with double && and so on), but couldn't.

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Cracktro :)

Post by albinopapa » October 27th, 2019, 10:06 pm

I want to address the trimTrailingSpaces function first.

Don't put the call to erase inside the loop, this is going to be error prone and buggy. Just get the starting offset to be erased so that after the loop, you can erase the entire range of trailing white space. Also, if you pass an invalid value to vector::erase() or string::erase() it will crash whereas passing a range of say trimStr.erase( trimStr.end(), trimStr.end() ) would not crash because of how for loops work, it just wouldn't do any deleting and consider the job to be completed and exit the function.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Cracktro :)

Post by albinopapa » October 28th, 2019, 12:26 am

Range based for loops
Sigh, I have to agree with you on sometimes or a lot of times also needing a counter or index of some kind and I mostly end up just creating an int count = 0; before the loop. I'm not going to suggest you do this as even I find it a little forced lol.

Pointer arithmetic
So there is some logic in using pointers as oppose to indices, which is the same as using iterators in a sense. You increment the iterator or pointer and when you dereference them you are already at the position in the array whereas using an index means the math of ( sizeof( T ) * index ) must be done before accessing the object. However, you can do your own tests and find that it may be only slightly slower or the same speed as using indices. The reason being I believe is you are iterating over a contiguous range of memory and the prefetcher fetches the next 64 bytes ( on x86 cpus ) and stores it in CPU cache. This means the data in either route is already loaded and the whole index calculation is the difference between a + and a *. With pointer/iterator arithmetic you must add or subtract by sizeof( T ) as it is and with indices you multiply sizeof( T ) with the index.

Variable names
Yes, you take it too far. I had a hard time working with another person on this forum on a project or two because he would also use one or two letter names for local variables and sometimes function parameters. I've heard this advice before and it really doesn't matter to the compiler either way. One of the reasons for this advice is say you were writing a library to be used by thousands of people. You want the interface to be descriptive so users of your library know what values to pass to functions or what functions to call and so on. A lot of people believe in a lot of these cases, the users of the library shouldn't even worry or care about the implementation as long as it works. So having anything you want as variable names in the implementation shouldn't make a difference. Now let's say you want help on the library because you are not able to maintain it as often or the code base is getting large enough to need contributors. Now they have to decipher your short hand to determine how the code works before being able to check for bugs or see if they can re-purpose a function for something they are working on. Nothing like having multiple functions that do the same thing because either your or someone else can't decipher the original code. My eyes were way less strained in the newer version of the trimTrailingWhiteSpaces function.

Code spacing
If you look through chili's framework there are more than a few places where lines of code just run together even after comments, so I can see where people might find this the norm and it probably is. While I know we write code that is sequential in nature, I still try to space out my lines of code based on variable usage. Like two or three lines to calculate a single variable then a line space then maybe an if statement. In the case of using int count = 0; before a range based for loop, I'll precede that line with a line space keeping the count and loop code clumped together

Code: Select all

// Some line of code not directly related to count or the following loop

int count = 0;
for( auto const& thing : things )
{
     // do something with thing and count
}

// Some code using count after the loop
To me this makes finding sections of code within a long function easier to find as opposed to just a massive wall of text, and yes breaking functions into smaller helper functions will help.

Const Functions
Oh boy, this was a particular cause for headaches for me in the beginning and now that I've been doing it for awhile I can't even remember why these were so troublesome. I'd start writing const functions only to have to undo a bunch of const declarations because it didn't fit what I was trying to do. Then one day literally it just clicked. Only declare functions that DON'T MODIFY THE OBJECT as const and only put non-modifying code in const functions. Breaking up functions to only having one responsibility really helped make this possible. You may have to retrain your brain, I did. For instance, if a Entity::draw() function's only responsibility is to draw the Entity, then there's no data to change and therefore can be declared as const. You can have an Entity::update() function to make any changes before drawing or even functions like setPosition() or whatever if some external function needs to modify the object, but must be done outside the Entity::draw() function.

Constructors
I've tried the different ways of initializing objects as well and constructors to me are so much cleaner. You get to create an object that is ready to use right away instead of create the object then initialize it somewhere later on in the function. One thing you can do with constructors that you can't do with Init functions is allow your class to store a reference to an object instead of forcing you to store a pointer. There are limitations with storing a reference, but through a little experimenting you'll quickly find them and probably get frustrated :p. One of the benefits of using Init functions I suppose is delayed initialization which is handy when you can't initialize an object right away in the constructor of another class because it relies on loading data from a file for instance. I've found a way around this by just creating a factory function that returns the initialized object after reading in the data.

It seems counter intuitive, but this factory function could create temporaries as the data is loaded then the final return statement would be returning the object with the temporaries being passed in as constructor values.

Code: Select all

WindowWidget WindowWidgetFactory( std::string _filename )
{
     auto file = std::ifstream( _filename );
     if( !file.is_open() ) 
          throw std::runtime_error( "File not found" );

     int width = 800, height = 600;
     file >> width >> height;

     bool hasBorder = true;
     file >> hasBorder;

     int borderThickness = 3;
     if( hasBorder )
          file >> borderThickness;

     return WindowWidget{ width, height, borderThickness };
}

Game::Game()
     :
window( WindowWidgetFactory( "window.settings" ) )
{}
A lot of my code examples are going to be game related lol, I haven't done much else with C++.

Smart pointers
If you can use std::vector, you can use std::unique_ptr. If you find both intimidating, you're probably referring to the template syntax. This was my hangup with starting to use std::unique_ptr. I first used new/delete and hesitated to use std::vector before std::unique_ptr was implemented or before I knew it had been implemented. By the time I found std::unique_ptr, I had been using std::vector for about a year so it shouldn't have been a big deal, but I didn't know how to initialize them and before C++14 you had to call new anyway. Once C++14 introduced std::make_unique, which is not concise to me as make_unique means to make something unique or unlike something else not "make a unique_ptr", I hesitated again because of the name and how odd it felt having to call a factory function to make a unique_ptr. It probably wasn't until I learned a little about templates that I started embracing unique_ptr and make_unique, so about 2015 or so.

String vs C strings
In C++17 there's a new player called std::string_view which seems to actually be the new go to for passing strings around to non-owning functions. This way if you pass a string literal ( "Hello, world!" ) or an std::string or even a C string and size, you get the same functionality of std::string without having to copy a C string to an std::string. You get iterators, random access using the subscript operator ( operator[] ), all the same find functions of std::string and so on. Std::string_view is a non-owning view of a string so keep that in mind. It basically stores a pointer to the char array. I need to start using this one myself. It's been a part of the STL for 2 years now and I'm still using std::string SMH.

Passing parameters instead of storing them
In regards to passing the Font by const&, I'm referring to you storing a pointer to a Font in Graphics and Sprite, not so much on how you pass the Font though by const& is my preferred method over pointer. This goes back to the single responsibility idiom with classes this time. The Graphics class's responsibility is to draw something that will be displayed on screen whether it be a procedurally generated shape or a sprite or some text. The text does need a font, but having to get the old font, set the new font draw the text then restore the old font when wanting to use different fonts seems like a lot of wasted effort when you could just simply pass the different fonts to each call to a drawString() function. A HUD class or UI class would seem like a more legit place to actually store your fonts and just pass them along to the drawString() function. I think since you stored the font and wanted to be able to change it, it forced you to use a Font* instead of a Font& or Font const&.

Constexpr vs macros
Hate to disappoint, but #defines are considered macros even if they are just defining constants. One of the things I like about using a constexpr value instead of a macro is during debug I can hover my mouse over a constexpr variable and get it's current value, but Visual Studio doesn't do this for macros while debugging. So basically a macro is separate from the C++ language, it's like it's own language. When compiling your source code there are phases it goes through. First comments and white space might be removed, then every #define'd thing ( constants or function macros ) is copied and pasted wherever they are used after the #define. Constexpr values are pretty much the same thing just not handled by the preprocessor phase, but instead by a later phase like the compiler or linker or optimizer. Macros pollute the global namespace which may not seem that big a deal for smaller projects, but as the code grows or if you add a third party library you may run into name collisions. You can put constexpr variables in namespaces

Code: Select all

namespace Globals{ constexpr float pi = 3.1415962f; }
float radians = degrees * ( Globals::pi / 180.f );
you notes
Yes, sorry, didn't think that one through, the second part of the second bullet is a continuation not a new bullet.

<random>
The suggestion to use the <random> library is one of convenience IMO. With the C rand() function you mostly use it with % to constrain it's range and it only goes from 0 to 0x7FFF ( 16 bits ). Both means you must do a lot more work for random values for larger types like 32 bit integers, get floating point values or have random negative numbers. With the C++ random library, you have different random number generators you can use as well as different distribution methods that support larger integer types that can be negative or floating point types like float or double, it isn't just a because C++.

<chrono>
The <chrono> library suggestion is kind of a nicer way of handling timers like calculating elapsed time in different periods like milliseconds or microseconds. I haven't personally used the std::chrono::system_clock yet, but cppreference.com does have examples on using it.

auto
One of the biggest push backs against auto is the programmers lack of knowing what the type is. Declaring something auto and declaring something explicitly have almost no effect on the code, but auto does hide the type requiring some investigation on what the type could be. In those cases declaring something explicitly as int or float or MyWidget can be helpful. Most say to use the "Almost Always Auto" idiom while others have found cases against using auto and can produce unexpected results. The general rule of thumb then becomes, if you don't care about the type being used, then use auto otherwise explicitly declare the type. There's another idiom called "Don't Repeat Yourself" which using auto allows you to do:

Code: Select all

auto windowWidget = WindowWidget( width, height, thickness );
or 
WindowWidget windowWidget = WindowWidget( width, height, thickness );
I guess this works too in this example:
WindowWidget windowWidget{ width, height, thickness };
Something I just thought of about why I would personally choose the auto windowWidget version over the 3rd version. In Visual Studios, if you use the first or second versions, you can mouse over the constructor and intellisense will display a tool tip window with the parameter list, so you can check if you got the order of all three ints correct, whereas the third version hovering over WindowWidget just shows if it is a class or struct and hovering over windowWidget just tells you it's a variable of type WindowWidget.

I understand all your hesitations, I've been through probably most of them myself. You'll have to go at your own pace and you might have to learn one part of the language to understand another part of the language.

You mentioned you didn't like cppreference.com and I also had an aversion to that site and used cplusplus.com for the longest time because I liked the layout a lot better and the examples were better. Unfortunately, it seems they stopped updating it after C++11, so I had to learn to navigate and understand cppreference.com. As stated previously, I do not like taking advice from stackoverflow since different people have their own ideas on how to accomplish the same thing and as you said, you might not know what is C code, what is C++ code and if the code is "good practice". It does have it's place though. Once you understand what their code does, you can massage it to fit your style and even use some STL algorithms that would reduce the code you need to maintain and bug check.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

Byteraver
Posts: 60
Joined: May 19th, 2016, 6:48 am
Location: Belgium, Europe

Re: Cracktro :)

Post by Byteraver » October 29th, 2019, 8:35 am

Hi again! Sorry for the late reply but I was not home yesterday night. Thanks, once again, for your elaborate answer. I shall try to take your good advice to heart, even though I know old habits die hard, whether they be good or bad :) Don't kill me if you notice a relapse occasionally ;) Also, refactoring might take a while. I did make a slightly better trimTrailingSpaces though:

Code: Select all

std::string& XMLFile::trimTrailingSpaces( std::string& trimStr )
{
    size_t strLen = trimStr.length ();
    for ( ; strLen > 0; strLen-- ) {
        if ( trimStr[strLen] != ' ' )
            break;
    }
    trimStr.erase ( strLen );
    return trimStr;
}
It seems pretty solid. Now my question on this is, if this would make the compiler use move semantics?

I'm off to work now, I shall try to write more later on :)

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Cracktro :)

Post by albinopapa » October 30th, 2019, 3:08 am

Still don't think that's going to work. The std::string::erase() function has two versions and they both take iterators. One versions says, delete the element that the iterator I give you points to, and the other says delete all the elements from the first iterator I pass to the last iterator I pass.

std::string::erase()

Basically, you'll want to erase a range, from the last non-white space character to the end of the string.
trimStr.erase( trimStr.begin() + strLen, trimStr.end() );
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

User avatar
chili
Site Admin
Posts: 3948
Joined: December 31st, 2011, 4:53 pm
Location: Japan
Contact:

Re: Cracktro :)

Post by chili » October 31st, 2019, 10:18 am

std::string::erase (and other such functions from basic_string) has a version that works with indices instead of iterators. In this it is distinct from the 'container' types such as std::vector
Chili

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Cracktro :)

Post by albinopapa » October 31st, 2019, 1:15 pm

Wow, I completely overlooked that version

Code: Select all

basic_string& erase( size_type index = 0, size_type count = npos );
Sorry Byteraver, for doubting.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

Byteraver
Posts: 60
Joined: May 19th, 2016, 6:48 am
Location: Belgium, Europe

Re: Cracktro :)

Post by Byteraver » November 1st, 2019, 10:01 am

I'm a bit relieved to see that the fact that my trimTrailingSpaces function actually works fine is not a coincidence or an exception waiting to happen ;) I did test it with zero as a parameter and it gave no error in debug mode so I trusted it to be fine. I considered using your version instead still because I have more confidence in your coding prowess than in mine, obviously, but hesitated because I don't like to use code I do not yet fully understand.

You put a lot of effort in this post and again, I'm very grateful for that. It is a lot of information / knowledge to take in, it will require some time. But I feel it is the right time for a push in c++ knowledge. The material in this post should keep me busy for the foreseeable future :D :D
Thanks again, the forum would not be what it is without your considerable effort. Must be said.

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Cracktro :)

Post by albinopapa » November 1st, 2019, 4:30 pm

Hey, thanks, that means a lot byteraver.

I'm hoping to see something come of all this from you. Most of the stuff posted on this forum are arcade style games, I'd like to see someone make some serious progress on an RPG for once.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

Post Reply