Add more styling options to class wizard

[BlueHazzard]

Hi,
i have to make A LOT classes and the styling convention of the normal codeblocks settings are not in accordance with the code styling guidelines of the target, so added some settings to control the style of the generated code.
For this i had to add a settings dialog for the classwizard plugin.

You can find the changes on this branch:
https://github.com/bluehazzard/codeblocks_sf/tree/test/classwizard/addSettingsDlg


possible changes:

Code

* Parameter name of the set function:
  - use generic "var" name (default) 
  - use variable name without prefix
* First letter of set and get function
  - upper case (default)
  - lower case
* Implementation of set and get functions
  - header file (default)
  - source file
* function name generation for the set and get functions
  - variable name without prefix (default)
  - variable name but with first letter upper case (came case) after the set/get
  - variable name all lower case
  - variable name all upper case

additionally i added in the settings dialog the settings for the file extensions. Right now they are saved from the new class dialog. I added this for convenience...

any comments?
greetings

[oBFusCATed]

1. Always use the override keyword when overriding virtual methods
2. Don't commit commented code
3. Indent with 4 spaces
4. Prefer declaring enums for special options instead of using the plane numbers (I'm talking about nameGeneration, firstLetter, implementation)
5. Prefer using simple range based iteration instead of hard to read and easy to make an error while loops. I'm talking about this

Code

while (implementation == 1 && it != m_MemberVars.end() )

This could easily be replaced by an if and for-each loop.
6. Calling Right on empty string I think causes an assert
7. Do not write complex ternary operators, they are hard to read or debug. if-else are better.
8. Use const more often
9. Consider adding a function which can add a single row. The code dealing with code printing is quite hard to read.

I cannot comment if this is useful addition or not because I'm not using this feature.

[BlueHazzard]

1. Always use the override keyword when overriding virtual methods

never seen in cb code... Will do!

2. Don't commit commented code

Yea... i will fix this with a rebase

4. Prefer declaring enums for special options instead of using the plane numbers (I'm talking about nameGeneration, firstLetter, implementation)

Isn't is going to clash with the config read functions? (enum <-> int is not that easy anymore with never c++ compiler)
so i have to use static casts, but it seems to be the better solution instead of unchecked ints

Code

const ClassWizard::eFirstLetter firstLetter = static_cast<ClassWizard::eFirstLetter>(cfg->ReadInt(_T("/SetGet/FirstLetter"), 0));
[...]
        switch(firstLetter)
        {
        case ClassWizard::upperCase:
            mv.Get << _T("Get"); break;
        case ClassWizard::lowerCase:
            mv.Get << _T("get"); break;
        }

Is this coding style ok?

9. Consider adding a function which can add a single row. The code dealing with code printing is quite hard to read.

I do not understand this sentence.. What do you mean by "add a single row" ?

I will add a other option to make the set function use const ref as parameter.

[oBFusCATed]

4. Yes, you need to use casts from int and back to int. Currently I prefer to use constructor like casts, like mytype(myintvar). They make code easier to read. But are not possible to use all the time.
9. The code related to adding code to the editor/file is rather ugly and bad because you have

Code

<< bla << someotherbla << endofline << newlinebla << endofline << more

My suggestion is to do something like appendLine(...); appendLine(...); I think it will make the code easier to read.

[BlueHazzard]

ok, i rebased the branch and made a force push, so if someone wants to test it he has to force a pull...

i fixed most of your suggestions, beside the 9. If i add this i have to rework all code of the class wizard, and i do not have time and will to do it. I have tried it a bit, but the resulting code was not more readable as the current code... I think it is ok as it is, at the moment. I have added all things i need for my work. If someone else wants to add more functionality and change the code generation he can do it (maybe i am this person in a few months...) But i think the improvements ate the moment are quite good and necessary (specially the const ref setting).

[oBFusCATed]

I don't think you need to introduce the usage of the function in the whole plugin, just the new places. But this is up to you, you're reading and modifying this code. I doubt that I'd touch it for anything other but reviewing patches. 

[oBFusCATed]

1. Do not use virtual and override at the same time
2. Do not write code like for (auto x: container) prefer for (const MyType &x: container) there is no point hiding the type. I don't understand why they teach people to do it. Using just auto would do also a copy and I doubt you want to do it.
3. Do not directly cast an int to enum without checking if the range matches.
4. Do not prefix enum types with 'e'.
5. Write more comments what variables and functions do.

I guess you can push/commit this whenever you like...

[sodev]

2. Do not write code like for (auto x: container) prefer for (const MyType &x: container) there is no point hiding the type. I don't understand why they teach people to do it. Using just auto would do also a copy and I doubt you want to do it.

Do write for (const auto& x : container) if you dont modify the items, do write for (auto& x : container) if you do modify the items, do write for (auto&& x : container) if you intent to move the items away (or you want/need to be flexible in using case 1 or 2 and can live with the fact if after refactoring your container returns copies instead references that your code happily modifies the copies instead of the items undetected).

You use auto bascially for 4 reasons:

• Easier Refactoring: Less changes of places that stay syntactically the same, you dont need to change the places that define stuff or only pass it around, only the places that do something with the stuff directly
• Prevent unintented type conversions: unsigned int? size_t? container::size_type?
• Define types only on one side: std::shared_ptr<std::string> ptr = std::make_shared<std::string>() vs auto ptr = std::make_shared<std::string>()
• Less Writing: std::map<std::string, std::set<std::pair<std::shared_ptr<MyFancyClass>, unsigned int>>>::iterator item = container.find("foo") vs. auto item = container.find("foo")

[oBFusCATed]

@sodev:
1. this is a stupid reason, how do you refeature (there is no such thing as refactoring) a code you cannot understand because all the type information is hidden from you behind auto?
2. this rarely happens and it not a real reason and auto of course is quite confusing if you don't know the actual rules.
3. this is sort of valid
4. this leads to unreadable code for me - it is far better to use typedefs for the container and ::iterator.

[sodev]

@oBFusCATed
I don't want to derail this topic too much, this can lead to philosophical questions like tabs vs. space, but these range-based for loops are one of the best places to use auto imho, so i couldn't resist.

The key point is to write flexible, reusable code and to achieve that you don't "code against the type" but "code against the properties", and for this you don't need the concrete type. For example pointers, in most locations it doesn't matter if it's a raw pointer or a smart pointer (it only matters if you need to create or destroy it), you only need to know what you can do with it, like dereference.

Another example that touches point 1-3 is if you need to store the return value of a method. Something like that:

Code

unsigned int index = getIndex(const Item& item);

Somewhere in your code this method is defined and in that place its return type is defined. Now in this line you again define its return type. You define the same thing in two places, bad if you need to change the definition of the method someday because you need to change all the others places as well. And does it really matter to see in that line that index is an unsigned int? What if it is a size_t? uint64_t? std::vector<Item>::size_type? It is an index, you can increment it, decrement it, compare it against some capacity value, thats what you need to know.

Actually this simple line is one of the reasons why i can't port a quite sized codebase to run in 32 Bit or 64 Bit, the method returns a size_type which changes from 32 Bit to 64 Bit but the unsigned int stays, depending on platform, 32 Bit. With auto i wouldn't need to take care of these places.

[oBFusCATed]

The key point is to write flexible, reusable code and to achieve that...

I've stopped reading after this point, sorry. You just repeat what all those modern c++ people say.

[BlueHazzard]

Ok, i will look into this as soon as i find time and commit it with your comments fixed.

Code

(const auto& x : container)

is my preferred since some performance problems lately

4. Do not prefix enum types with 'e'.

Why?

[oBFusCATed]

I don't see a reason to hide the type in the ranged-based-for loops. If you like it fine. But if you do it in a code where I'm interested in looking I'll just change it.

4. Because it is ugly and I think types must start at least with capital letter according the the limited style guide. Also any form of Hungarian notation is stupid.

[sodev]

I don't see a reason to hide the type in the ranged-based-for loops.

Also any form of Hungarian notation is stupid.

I wouldnt have expected to see both sentences from one person in one post . Sadly it is only CodeBlocks that "hides" the type of auto variables, pretty much every other IDE shows you their type (hint, hint ).

[oBFusCATed]

@sodev: You see the type in notepad++, github, simple patches shown in text editors? I review a lot of code and using auto makes it a lot harder for me to do a good job.

BTW, even clangd didn't show the type for auto variables back when I've tried it last year. Hopefully they'll implement this someday.