FC Code Quality

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
chrisb
Veteran
Posts: 53930
Joined: Tue Mar 17, 2015 9:14 am

Re: FC Code Quality

Post by chrisb »

Zolko wrote: Fri Sep 23, 2022 12:37 pm yes I have, for example, when vox (if I remember right) wanted to re-factor all the Draft code to be more Python compliant, exactly like this white-space cleaner: now the code is even more unreadable and un-usable.
Agreed! But we see here much more substance.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
User avatar
chennes
Veteran
Posts: 3881
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: FC Code Quality

Post by chennes »

As someone who reviews PRs (including several of his), I believe that not only is @berniev acting in good faith, but he is willing to both do the grunt work of actually implementing his suggestions (he's not just complaining for the sake of complaining), and also work constructively to improve his own work. I believe we can all act in that spirit of cooperation -- his suggestion uptopic is not a "whitespace" change, or pure formatting, it's a move to objectively better code.

Of course right now is not a good time to make sweeping changes to code that is going to be touched by @realthunder's Toponaming code, simply because merging that branch is already going to be a tremendous amount of work for everyone involved and there's no call for us to go making it any harder when there are other places we can work. Focusing on improvements to the GUI code is a good idea, since that mostly avoids toponaming stuff, and there's a ton of work to be done there anyway.

It goes without saying that newly-written "Color" and "Position" classes probably aren't actually going to get merged, but in the sense that this is a "proof of concept" of the sort of improvement berniev has in mind I think it's a good step. Clearly real code is going to either use the Qt or Coin color and position classes, depending on its use -- that doesn't affect the underlying principle of the suggestion, which echos pretty much all modern coding advice. (On that note, though, I'd add a book to your reading list, berniev -- have a look at The Pragmatic Programmer by David Thomas and Andrew Hunt. In particular I think you may want to reconsider the sort of subclassing you demonstrate in your example file.)

I'd suggest that overall in the codebase there are a few major items to consider refactoring, if you are looking for maximum concentrated impact:
  1. Break up the megaclasses and megafiles
  2. Get rid of macros and replace with modern C++
  3. Modernize old C-style code
  4. Improve const-correctness
  5. Improve convoluted function signatures
Last, a piecemeal approach is far easier to review than making every possible change all at once (it also leads to less bikeshedding in the long run, since an objection to one sort of change doesn't block the application of everything else). So as a reviewer I ask that you consider submitting small-ish, highly-focused changes.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
berniev
Posts: 247
Joined: Wed Apr 13, 2022 10:45 pm
Location: Oz

Re: FC Code Quality

Post by berniev »

Thank you for the kind words.

This post is to demonstrate criticism of FC code is indeed justified. Maybe a little humility from time to time would not go astray. Many messengers get shot or bullied or dragged off-topic.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: FC Code Quality

Post by openBrain »

berniev wrote: Fri Sep 23, 2022 3:38 pm Thank you for the kind words.

This post is to demonstrate criticism of FC code is indeed justified. Maybe a little humility from time to time would not go astray. Many messengers get shot or bullied or dragged off-topic.
Feels humility there :roll:
berniev wrote: Sun Sep 18, 2022 10:10 pm
It's hardly respectful.

Even being right -- if it could be something absolute -- doesn't give you all rights. ;)
Post Reply