[PD] Pad/Pocket: sort out duplicated code

[donovaly]

the checkboxes are almost identical and it avoids work to have them in one location

[freecadci]

for feature branch PR_5355. Pipeline 443882266 was triggered at 5345e58. All CI branches and pipelines.

[wwmayer]

TaskExtrudeParameters is the base class of TaskPocketParameters and TaskPadParameters. Now using in a class stuff from derived classes is not a great idea.

What you can do instead is moving the enum class Modes of TaskPocketParameters and TaskPadParameters to TaskExtrudeParameters. Now there is one enum entry different of the two classes and you can solve it this way:

    enum class Modes {
        Dimension,
        ThroughAll,
        ToLast = ThroughAll,
        ToFirst,
        ToFace,
        TwoDimensions
    };

This way you don't need to have two variables modePad & modePocket but only one.

[donovaly]

This was my first attempt but since the 2 enums are different for Pad and Pocket, I did not find a suitable solution. Through all gets the "through all" length and this is longer than when to the last face. Therefore the 2 enums are different.

Maybe I have a think here? If not, is there another way?

[wwmayer]

This was my first attempt but since the 2 enums are different for Pad and Pocket, I did not find a suitable solution. Through all gets the "through all" length and this is longer than when to the last face. Therefore the 2 enums are different.

Sorry, but I don't get it.
Currently we have the two enums

    enum class Modes {
        Dimension,
        ToLast,
        ToFirst,
        ToFace,
        TwoDimensions
    };

with Dimension=0, ToLast=1, ToFirst=2, ToFace=3, TwoDimensions=4 and

    enum class Modes {
        Dimension,
        ThroughAll,
        ToFirst,
        ToFace,
        TwoDimensions
    };

with Dimension=0, ThroughAll=1, ToFirst=2, ToFace=3, TwoDimensions=4. The only difference is ThroughAll and ToLast but because both have the same value a combined enum class can be created:

    enum class Modes {
        Dimension,
        ThroughAll,
        ToLast = ThroughAll,
        ToFirst,
        ToFace,
        TwoDimensions
    };

with Dimension=0, ThroughAll=1, ToLast=1, ToFirst=2, ToFace=3, TwoDimensions=4. So, the important thing is that all enum labels still have the same value and ThroughAll has the same value as ToLast.

[donovaly]

Through all gets the "through all" length and this is longer than when to the last face. Therefore the 2 enums are different.

Maybe I have a think here? If not, is there another way?

;-) well, this was already my thinko. That the modes do something different does not mean they need a different index.
So yes, sure, the enum can be sorted out to the helper class too. I updated the PR accordingly and I think the PR should now be ready to merge.

[wwmayer]

It may sound nitpicking but the boolean here is a code smell (called boolean blindness) because in places where the function is used you see this setCheckboxes(index, true); or this setCheckboxes(index, false); and for a reviewer it's not immediately clear what the intention is, hence boolean blindness. So he has to go to the class declaration to get its meaning.

A better way is to use an enum that can be declared as:

enum class Type {
  Pad,
  Pocket
};

Then in the code you will have to write setCheckboxes(index, Type::Pocket); or setCheckboxes(index, Type::Pad);

[donovaly]

It may sound nitpicking but the boolean here is a code smell (called boolean blindness) because in places where the function is used you see this setCheckboxes(index, true); or this setCheckboxes(index, false); and for a reviewer it's not immediately clear what the intention is, hence boolean blindness.

I can change this to an enum. However I don't understand the term "blindness". I named the Boolean "isPocket", so it is clear what it is about. The IDEs like KDevelop, MSVC etc. show you this info while typing. Also when you right-click on it, you get the info about the procedure definition. If I would have added a comment in the .h file, the IDE will also show me this. So with one click, the coders get the info.
When you don't have an IDE but use a text editor, then I agree an enum helps for readability.

I must admit I use bools because this is quicker ceded than adding an enum, since I write a procedure and in the IDE I can right-click on it to ad its definition in the header file. Therefore I only need to write once, the rest is automatically done by the IDE. (Only the position in in private/public etc. needs sometimes to be adapted.)

So should I in future prefer enums over bools?

[donovaly]

The enum is now in.

[wwmayer]

I can change this to an enum. However I don't understand the term "blindness". I named the Boolean "isPocket", so it is clear what it is about.

It only becomes clear if somebody who reads the code switches to the class declaration. And it's only clear if the value is true, but what if it's false? Somebody who is not familiar with the code won't know that it will be a pad then.

To better understand the issue have a look at: https://doc.qt.io/archives/qq/qq13-apis.html#thebooleanparametertrap

I must admit I use bools because this is quicker ceded than adding an enum, since I write a procedure and in the IDE I can right-click on it to ad its definition in the header file.

But you don't write the code only for you but for everybody else. And thus the client code should be as clear as possible so that there is no need to always jump between client code and class declaration.

Then with a boolean you only have two cases but what if in the future a new case will be added? Do you then add another boolean? An example of this bad practice is

FreeCAD/src/Mod/PartDesign/Gui/ReferenceSelection.h

Line 50 in f1d6c78

const bool edge_, const bool plane_, const bool planar_,

which in client code then e.g. looks like this

FreeCAD/src/Mod/PartDesign/Gui/TaskDraftParameters.cpp

Line 215 in 249c2c7

Gui::Selection().addSelectionGate(new ReferenceSelection(this->getBase(), true, true, true));

Having six boolean parameters makes it really difficult to understand the code immediately.

[donovaly]

To better understand the issue have a look at: https://doc.qt.io/archives/qq/qq13-apis.html#thebooleanparametertrap

Thanks, interesting article.

An example of this bad practice is

FreeCAD/src/Mod/PartDesign/Gui/ReferenceSelection.h

Line 50 in f1d6c78

const bool edge_, const bool plane_, const bool planar_,

I see.
I think the point is that everybody uses an IDE with proper information delivery. For example here with MSVC I just have too move the mouse over a 'true' and get its type and when I want also the whole procedure definition. But it is exactly this feature doesn't make you think about code readability for those without a proper IDE. I'll try to think about this more.

[wwmayer]

An IDE doesn't help at all when reading the code on-line.

[freecadci]

for feature branch PR_5355. Pipeline 446453075 was triggered at f473f3b. All CI branches and pipelines.

[freecadci]

for feature branch PR_5355. Pipeline 447017160 was triggered at 513420d. All CI branches and pipelines.

[wwmayer]

This doesn't make much sense. Why is index passed twice now?

In my code example the second argument was supposed to be new enum class type:

enum class Type {
  Pad,
  Pocket
};

The called function would then be: setCheckboxes(index, Type::Pad); and of course the signature of setCheckboxes is: void setCheckboxes(int, Type);

[donovaly]

This doesn't make much sense. Why is index passed twice now?

Good question. I have no clue why I force-pushed this. Before my last push the enum was correctly passed.

I see now that you already fixed and merged. Many thanks.

[wwmayer]

ToLast is equal to ThroughAll and thus this else block is dead code. So, what's still needed is the extrude type (Pad or Pocket) to make sure ThroughAll is only handled for Pockets and ToLast only for Pads.

[wwmayer]

See 66864bb