Refactoring Sketcher 2

[AjinkyaDahale]

Proposal description

This will be the continuation of my previous work, as described in #6. While a lot of progress has been done, this is by it's nature a perpetual project. In this stage of the work, I aim to move on to the larger refactorings required to make Sketcher more maintainable and extensible.

Deliverables

Most of the previous deliverables are still relevant now:

• Simplifying methods: There has been a lot of progress in terms of the reducing the cognitive complexity of the most offending functions, particularly in SketchObject.cpp. However, there are still many methods well above our threshold of 25, with some having complexity in the hundreds. Most of the changes have been relatively safe manipulation of for and if blocks, but further changes will need comprehensive testing in place.
• Testing: A set of detailed tests have been added for a few methods, including getPoint, deleteGeometry, split and trim, and these tests along with refactored methods are now merged ([Sketcher] Round 1 of refactors FreeCAD#15811). The biggest challenge in testing is the large number of cases possible, even for the simplest operations.
• Documentation: Admittedly, this has had the least progress the last iteration. I expect that as the code starts getting reviewed and merged (and as I myself review others' code), there will be enough content to be compiled into a set of "best practices".

In addition, this project will have the following targets:

• Merging into main: Due to the feature freeze in place for FC 1.0, a lot of other work on Sketcher is also waiting to be merged. As new features get merged, there may be a lot of conflict resolution. This is not expected to be a major part of this project, but still might take some time. Additionally, future work will have more focus on immediate mergeability to ameliorate this issue. This would involve a comprehensive testing framework.
• Help with review of other PRs: If needed, I'll also help out with conflict resolution in other Sketcher PRs.
• Reduction of file size: As the refactoring makes code more contained, there can now be a focus on moving some code outside the 10,000+ line long files. For most of the files mentioned earlier, this can be done relatively easily by simply moving classes to separate files. However, SketchObject.cpp (and to a lesser extent Sketch.cpp) could be more challenging and interesting. My current plan is to keep SketchObject as a container for all the geometries, and move methods that act on them (like deletion, addition, splitting, trimming etc) into separate classes, following the single responsibility principle.

Timeline

To an extent the work is already ongoing. However, I have a deadline approaching in my day job, so nominally we can say this work can start from 15th of December, 2024.

I would be dedicating about 10--40 hours per week for this project, depending on other obligations. In total, I aim to complete the total hours asked for this project by March 2025.

Risks and mitigation

I am currently working in a part-time capacity at IIT Bombay (specifically, on the Osdag steel structure design tool), so the time I can allot to FreeCAD may be limited and variable week-to-week. That said, my role at IIT Bombay also involves FOSS, and the project authorities are more than happy to accommodate my schedule.

As I mentioned earlier, by it's nature this is a perpetual project, and there will always be room for improvement. However, in the long run, maintaining clean code is the responsibility of everyone involved in developing it. I expect that this job will be easier once a document about best practices is in place.

Compensation

I would like a compensation of 40.00 EUR/hour, up to a maximum of 100 hours of work, i.e. 4000.00 EUR.

About you

My name is Ajinkya Dahale. I go by jnxd on the FreeCAD forum and AjinkyaDahale on Github. I am a software developer based in India, and a contributor to FreeCAD since 2016.

This is the second "stage" in the refactoring project. I learned a lot during the previous iteration, and should be more effective this time around.

[Reqrefusion]

I think you gave some wrong links. Issue 21 is irrelevant and the Osdag link you gave is not going anywhere. Here you have summarized the previous work in general terms; in fact, it is not necessary since everything is in a single PR, but it would be great if you prepare an FPA report. Problems you encountered etc. You can explain it in more depth.

[AjinkyaDahale]

I think you gave some wrong links. Issue 21 is irrelevant and the Osdag link you gave is not going anywhere.

Fixed. I had used the correct link for Osdag but apparently the https:// is important.

Here you have summarized the previous work in general terms; in fact, it is not necessary since everything is in a single PR,

PR #15811 is just the first of the changes I intended to push as part of the previous work. #14696 is the entire thing, and as such, it probably should not be a single PR.

but it would be great if you prepare an FPA report. Problems you encountered etc. You can explain it in more depth.

There are reports in https://github.com/FreeCAD/FPA/tree/main/reports/SketcherRefactor. You may want to read the files as "code/raw": I didn't quite get the table right. It's probably not as detailed as you may like, so if you have specific questions I could add them. The biggest problems as such are the sheer size of test cases, as well as merging while other code is being developed and merged.

[yorikvanhavre]

This looks good for me! Would you consider also adding "testing" to point 2: Help with review of other PRs: If needed, I'll also help out with conflict resolution in other Sketcher PRs.? I mean, specifically that we could ask you to test PRs...

[PaddleStroke]

I personally would prefer a project that aim at fixing bugs and that do refactoring along.

If it's refactoring things like trim, rebuildexternalgeos etc. Then I think it's worth doing.

Edit: removed undue rant on get point since I should have raised this in the PR before.

[Reqrefusion]

Here you have summarized the previous work in general terms; in fact, it is not necessary since everything is in a single PR,

PR #15811 is just the first of the changes I intended to push as part of the previous work. #14696 is the entire thing, and as such, it probably should not be a single PR.

but it would be great if you prepare an FPA report. Problems you encountered etc. You can explain it in more depth.

There are reports in https://github.com/FreeCAD/FPA/tree/main/reports/SketcherRefactor. You may want to read the files as "code/raw": I didn't quite get the table right. It's probably not as detailed as you may like, so if you have specific questions I could add them. The biggest problems as such are the sheer size of test cases, as well as merging while other code is being developed and merged.

Yes, I have seen these, but the closest one was 3 months ago. I'm talking about something more like the last word. But the second PR mentioned in the reports still remains as a draft. So we cannot say that his first work is finished. So where does this work begin? Or where exactly does Refactoring number one end? I can't understand exactly what it covers and what it doesn't (This could probably be due to my ignorance). Let me generalize what Paddle asked and deleted and ask: Is reducing cognitive complexity the only value you consider in this study?

[AjinkyaDahale]

I personally would prefer a project that aim at fixing bugs and that do refactoring along.

Bugs keep coming, especially for sketcher. Every time they are in a new place and every time I (or whoever else is handling them) has to spend time looking for the issue, and often it is a 1-2 line fix. Refactoring aims to ease that process somewhat. Additionally, there is a lot of repeated code, and a bug-fix (or new feature) there means repeating the same thing at multiple places, possibly in the future.

I do agree that going down the line we shouldn't be needing refactoring as much, but at that time as well, we will have to ensure that new code complies with certain "cleanliness" standards.

If it's refactoring things like trim, rebuildexternalgeos etc. Then I think it's worth doing.

It can become an arbitrary metric very fast. But as of now, there are a whole bunch of methods that need refactoring.

Edit: removed undue rant on get point since I should have raised this in the PR before.

Your rant on getPoint may be valid, but I believe that there is not much harm in having it either way. We can continue the discussion on that topic somewhere else.

[AjinkyaDahale]

Yes, I have seen these [reports], but the closest one was 3 months ago.

Indeed the last report was 3 months ago, which was around the time when I started the job at IITB. That, along with other factors, has kept me from doing any further work. Mostly I had just been maintaining the PRs. I expect that as soon as the installer for Osdag is complete in the coming week (the deadline I mentioned in the proposal), I will have enough time to dedicate to this work.

I'm talking about something more like the last word. But the second PR mentioned in the reports still remains as a draft. So we cannot say that his first work is finished. So where does this work begin? Or where exactly does Refactoring number one end? I can't understand exactly what it covers and what it doesn't (This could probably be due to my ignorance).

Could you elaborate on what you mean by the last word?

As such, the last project (like this one) was a time based one. At any point, so long as the code compiles and doesn't add new bugs (which admittedly the trim refactor did in a transitional state), and the changes have made the overall code cleaner, we can call the work "finished" (and of course only bill for the hours spent till then).

I assert that by the end of the first work, I had put in the stipulated time (about 160 odd hours in total over the four months) towards the refactoring. The draft PR, I believe, fills in the aforementioned boxes, but I can't be certain for the lack of tests for many methods. That said, the parts that do have tests were not merged because of the feature freeze. I would count merging them as part of the first work, provided no major changes need to be done.

Let me generalize what Paddle asked and deleted and ask: Is reducing cognitive complexity the only value you consider in this study?

It is not perfect, but it still provides an objective and quantitative measure. Without an objective measure, we can be arguing forever as to the best way to keep the code. There are some methods that I left just above the threshold at 26 because the only opportunity I saw to reduce the number felt like a cop-out solution not really improving readability.

Admittedly, I was being overzealous about this metric, but at this stage, there are plenty of methods with cognitive complexity in the 100s, and reducing that does improve the code by whatever definition you use. Going forward, I would suggest a more fuzzy approach: e.g. do not bother until a method reaches a complexity of, say, 100; but if and when it does, reduce it down to, say, 20.

[AjinkyaDahale]

This looks good for me! Would you consider also adding "testing" to point 2: Help with review of other PRs: If needed, I'll also help out with conflict resolution in other Sketcher PRs.? I mean, specifically that we could ask you to test PRs...

Sure. Additionally, (as paddle must have already suffered) I would like to insist that new PRs also involve a little bit of refactoring wherever possible, or at least that they do not add to the complexity.

That said, I may also not be adept at understanding every PR put on my plate, especially if the code has changed significantly since I last worked on it (things related to OVP come to mind). In other cases, I might not have the time to do so before someone else can.

[Reqrefusion]

That, along with other factors, has kept me from doing any further work.

Yes, it is really difficult to know what the future has in store for us. I personally don't see this as a problem.

Could you elaborate on what you mean by the last word?

A declaration that your work is done. And the general considerations that make this declaration meaningful. General difficulties experienced during the project and future wishes. Paperwork done by the action hero in the office in American movies :D

As such, the last project (like this one) was a time based one. At any point, so long as the code compiles and doesn't add new bugs (which admittedly the trim refactor did in a transitional state), and the changes have made the overall code cleaner, we can call the work "finished" (and of course only bill for the hours spent till then).

Yes, the fact that it is a time-bound project creates a bit of confusion. But I do not think it is right to sit on such an account. If the second PR you mentioned in the previous report is accepted and a final report related to the project is written, it would be better to consider everything as completed.

That said, the parts that do have tests were not merged because of the feature freeze.

It's hard not to agree with this, feature freeze made the process difficult.

It is not perfect, but it still provides an objective and quantitative measure. Without an objective measure, we can be arguing forever as to the best way to keep the code. There are some methods that I left just above the threshold at 26 because the only opportunity I saw to reduce the number felt like a cop-out solution not really improving readability.

Ultimately there needed to be a scale to see progress. It's also great to mention that you took readability into consideration. No matter what value is taken into consideration, rewriting something provides an improvement because it gives people a choice. The question here is how much of an improvement there is.

[AjinkyaDahale]

That, along with other factors, has kept me from doing any further work.

Yes, it is really difficult to know what the future has in store for us. I personally don't see this as a problem.

Don't see this as a problem in what sense?

Could you elaborate on what you mean by the last word?

A declaration that your work is done. And the general considerations that make this declaration meaningful.

I think a fair stopping point for the previous work would be merging of changes in methods that have a good amount of tests ready. I don't have the list handy but I can edit this comment this weekend.

I'll address the other parts of the comment as well this weekend. The general idea however is that I can't provide much more than a few sentences towards the report you're asking for (in addition to the reports already submitted). As for the PR, I'm hesitant to merge code that doesn't have enough associated tests, and adding tests is by itself plenty of additional work, which is exactly what this present request is for.

[Reqrefusion]

Don't see this as a problem in what sense?

It made sense when I wrote it, but yes, it can be misunderstood that way. What I'm trying to say is that I accept that such things can happen, and I see the delays and problems within them as part of the process, not as a problem. I'm sorry if you understood it in a hurtful way.

I'll address the other parts of the comment as well this weekend. The general idea however is that I can't provide much more than a few sentences towards the report you're asking for (in addition to the reports already submitted). As for the PR, I'm hesitant to merge code that doesn't have enough associated tests, and adding tests is by itself plenty of additional work, which is exactly what this present request is for.

In the report, you can write that the study could not be completed because there was not enough time to write the tests required for the study and that you made a complementary offer because these were necessary for this study. So at least that's my understanding. This is understandable, after all, you made a time-bound offer. Of course, it had goals, but it was not a proposal to achieve them. Here is the following result. Is the offer you are making now a continuation or a complementary one? If it is both, how much of a continuation is it and how complementary is it?

Forgive me, I'm asking more administrative questions rather than technical details, I hope I'm not bothering you.

[AjinkyaDahale]

It made sense when I wrote it, but yes, it can be misunderstood that way. What I'm trying to say is that I accept that such things can happen, and I see the delays and problems within them as part of the process, not as a problem. I'm sorry if you understood it in a hurtful way.

Thanks for clearing it out. There was a slight ambiguity in the statement, but I understand.

In the report, you can write that the study could not be completed because there was not enough time to write the tests required for the study and that you made a complementary offer because these were necessary for this study. So at least that's my understanding.

Your understanding is correct.

Is the offer you are making now a continuation or a complementary one? If it is both, how much of a continuation is it and how complementary is it?

The basic idea is that whatever changes were made by the end of the last period should not have further costs associated with merging, even if there is conflict resolution involved. However, I would like to consider writing the tests themselves as a separate piece of work. It involves getting a proper idea of what a method is supposed to do, and particularly how it behaves with corner cases: for e.g. does it throw an exception or return a default value.

To that extent, I would suggest considering the PR FreeCAD/FreeCAD#18665 as "complementary" (by your usage), except for the commit "[Sketcher][test] Add tests for B-spline operations", which took a couple of hours for me to prepare last week.

In future PRs relating to this topic, as tests are written more commits of FreeCAD/FreeCAD#14696 (as of 1st September, 2024) will be added. I would consider the tests as well as any further changes in the code (not including conflict resolution) as a "continuation". Does that sound fair?

Forgive me, I'm asking more administrative questions rather than technical details, I hope I'm not bothering you.

Your questions are probably very much justified. However, I must admit writing reports feels like way more effort than writing code.

[chennes]

The Grant Review Committee supports this proposal, and it has been forwarded onto the FPA for voting.

[chennes]

The FPA has voted to fund this proposal. Please work with the FPA administrative team to create a contract as described here: https://github.com/FreeCAD/FPA/blob/main/handbook/process/fundsaccess.md#non-grant-contractors