Author Topic: small refactoring in findreplacedlg  (Read 8022 times)

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
small refactoring in findreplacedlg
« on: January 16, 2016, 01:28:12 pm »
hi everybody,

just some minor refactoring in this class:

- change some ints to size_t
- added some new lines for clarity
- added some conts in variable declarations
- changed IsEmpty() to empty() member function on wxStrings
- refactrored function FindReplaceDlg::FillComboWithLastValues:
    - removed loop and replaced it with member function which adds a wxArrayString
    - removed check for empty strings as it is not necessary. when the values get written into config
      this check is already performed
- refactored function FindReplaceDlg::SaveComboValues:
    - changed order of actions. instead of first adding the entries and then checking for the currently
      selected entry an d adding it, and removing it... , now it first gets the current selected value and
      adds it, and then goes through adding all other entries, skipping empties and doubles


as always: just refactoring. I sometimes use my low quality free time while traveling or sick, to just randomly jump into some file of c::b and read it, checking for strange things, or things that are hard to read at first glance, or that could hide some error, or some superfluous code...

please don't scold me for it. this is my free time  ;) and I can't spend it for things that would require a better programmer. making this small clean-ups eventually contributes a little bit every time to make c::b even better than it is  ;D

regards

frithjof
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: small refactoring in findreplacedlg
« Reply #1 on: January 17, 2016, 11:09:25 am »
To ensure that such things can be applied, please respect our coding guidelines.

In the snippet I would recommend:
  • Don't use STL methods on wx containers. The reason is that is is confusing if used in a mixed fashion, although it will do the same thing. We've started with wx (because there were no STL methods) so we would like to stick with it.
  • after if, for (...) statements insert a space before the opening bracket.
  • Things like:
    const unsigned int item_count = std::min(combo->GetCount(), 10u);
    ...should be commented. Because its is not clear in the first place why you picked "10".
  • Usually, when calling a method in a bracket construct (so its not a variable) you should place spaces around it. So this:
    if (!dir.IsEmpty())
    ...becomes that:
    if ( !dir.IsEmpty() )
    ...but this is OK:
    if (!bool_var_no_method_call)
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
Re: small refactoring in findreplacedlg
« Reply #2 on: January 17, 2016, 03:04:14 pm »
ok, I will do the according changes...

just one question: the value of maximally 10 entries written from the wxComboBox into the config was already there before my changes. there was no comment as to this value there neither. I will just add some generic comment then, because I don't know the exact reason why it was put there by the original author. I suppose it was meant to limit the amount of data in the config, but I am not sure...
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline frithjofh

  • Regular
  • ***
  • Posts: 376
Re: small refactoring in findreplacedlg
« Reply #3 on: January 17, 2016, 03:24:45 pm »
the new patch replacing the last

changes made based on the considerations of morten

thx for the input  :)
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: small refactoring in findreplacedlg
« Reply #4 on: January 21, 2016, 11:36:21 am »
the new patch replacing the last
Looks great && applied.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: small refactoring in findreplacedlg
« Reply #5 on: January 21, 2016, 09:03:29 pm »
Why is this static -> "static const unsigned int max_value"?
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: small refactoring in findreplacedlg
« Reply #6 on: January 22, 2016, 08:30:16 am »
Why is this static -> "static const unsigned int max_value"?
Because it must not be instantiated every time you can this function. A member var would also have been fine, but its used only there, locally and not by other methods.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: small refactoring in findreplacedlg
« Reply #7 on: January 22, 2016, 08:00:11 pm »
I hope you know that in c++11 and on linux. Every local static variable is protected by a mutex/atomic.
I don't know if this is the case for static const variables, but it should be kept in mind when using static.
Especially when the variable is not a basic type.

For this case it probably doesn't matter, but I just wanted to express my concern about this construct.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: small refactoring in findreplacedlg
« Reply #8 on: January 22, 2016, 08:07:17 pm »
For this case it probably doesn't matter, but I just wanted to express my concern about this construct.
OK, point taken.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ