Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AB3: Tracing Code] Add more headers to section instructions #42

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

UdhayaShan1
Copy link

@UdhayaShan1 UdhayaShan1 commented Jul 7, 2024

  1. Some existing images have arrows added.
  2. LogicSequenceDiagram puml edited to show creation of the parser and execution of EditCommand
  3. Introduce lower level sequence diagram of EditCommand under Tracing the execution path
  4. Add headers to segment the tracing to make it clearer
    image
  5. Add label annotations, green for method calls and red for returning from method calls.
    image

Fix #17

Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UdhayaShan1 PoC looks fine. You can go ahead.
BTW, tutorials/images/tracing/EditSequenceDiagramWithMainWindow.png is broken (plantUML error).

> Indeed, the ratio of time spent reading versus writing is well over 10 to 1. We are constantly reading old code as part of the effort to write new code. …[Therefore,] making it easy to read makes it easier to write.
>
> — Robert C. Martin Clean Code: A Handbook of Agile Software Craftsmanship

When trying to understand an unfamiliar code base, one common strategy used is to trace some representative execution path through the code base. One easy way to trace an execution path is to use a debugger to step through the code. In this tutorial, you will be using the IntelliJ IDEA’s debugger to trace the execution path of a specific user command.


## Before we start
<h2 id="before-we-start">Before we start</h2>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need. The anchor before-we-start is automatically generated by MarkBind. However, MarkBind will also warn about non-existent hash, which is a bug. Can ignore that for now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted

@UdhayaShan1 UdhayaShan1 requested a review from damithc July 13, 2024 05:47
@UdhayaShan1
Copy link
Author

Added remaining sections with edits reflected in PR description.

Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start @UdhayaShan1 Added some comments. Didn't have time to review everything. But you can go through the rest yourself and fix similar problems elsewhere.
Also remember to syn this branch with the latest master branch.

tutorials/ab3TracingCode.md Outdated Show resolved Hide resolved
tutorials/ab3TracingCode.md Outdated Show resolved Hide resolved
tutorials/ab3TracingCode.md Outdated Show resolved Hide resolved
tutorials/ab3TracingCode.md Show resolved Hide resolved
## Tracing the execution path

Recall from the User Guide that the `edit` command has the format: `edit INDEX [n/NAME] [p/PHONE] [e/EMAIL] [a/ADDRESS] [t/TAG]…` For this tutorial we will be issuing the command `edit 1 n/Alice Yeoh`.
At this point, you should have appreciated the general sequence diagram [**shown above**](#before-we-start)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions that might pop up in the reader's mind:

  • What do you mean by 'appreciated'?
  • What is a 'general sequence diagram'?

Missing a full stop.

Copy link
Author

@UdhayaShan1 UdhayaShan1 Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, you should have understood the high-level overview of the sequence diagram which shows the general execution flows between the main components [**shown above**](##setting-a-breakpoint).

We will begin to trace the following sequence diagram which explores the `Logic` component in more detail which will be paramount to understanding other commands as well!
The diagrams will be reproduced with labels in their sections to highlight the pass of control between classes.

Made it clearer that its high level sequence diagram with all components and general execution flow and shifted it below so we can link to the low level sequence diagram with explores Logic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to have these png file in the repo. MarkBind will generate it automatically from the puml file with the same name.

activate ecp
ecp -> abp
deactivate ecp
abp -> ecp ++: parse(1 n/Alice Yeoh)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
abp -> ecp ++: parse(1 n/Alice Yeoh)
abp -> ecp ++: parse("1 n/Alice Yeoh")

Check other places as well.


<annotate src="images/tracing/LogicSequenceDiagramWithMainWindow.png" width="900" alt="Sample Image">
<!-- Set Legend to both -->
<a-point x="24%" y="16%" label="1"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use a different label e.g., t1 to avoid confusing with step numbers in the explanations?

Copy link
Author

@UdhayaShan1 UdhayaShan1 Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure will call it T1

### 2. LogicManager -> AddressBookParser

<annotate src="images/tracing/LogicSequenceDiagramWithMainWindow.png" width="900" alt="Sample Image">
<!-- Set Legend to both -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<!-- Set Legend to both -->

Also update the alt property

<!-- Set Legend to both -->
<a-point x="82%" y="51%" label="5"/>
<a-point x="50%" y="64%" color = "RED" label="6"/>
<a-point x="26%" y="71%" color = "RED" label="7"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing around = is not consistent.

Copy link

@gongg21 gongg21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, good changes to the tutorial, just some minor tweaks to update accordingly.

tutorials/ab3TracingCode.md Outdated Show resolved Hide resolved
tutorials/ab3TracingCode.md Outdated Show resolved Hide resolved
tutorials/ab3TracingCode.md Outdated Show resolved Hide resolved
tutorials/ab3TracingCode.md Outdated Show resolved Hide resolved
tutorials/ab3TracingCode.md Outdated Show resolved Hide resolved
@UdhayaShan1 UdhayaShan1 requested a review from damithc July 27, 2024 06:06
@UdhayaShan1
Copy link
Author

UdhayaShan1 commented Jul 27, 2024

Thanks, changes made. Specifically,

  • removed redundant spacing
  • changed label names
  • fixed typos
  • removed css
  • removed png for puml that is not used for any annotation

Additionally, I added description to the labels as well to help make some of the execution flows clearer.

Branch is up to date with master as well.

@damithc
Copy link
Contributor

damithc commented Jul 27, 2024

@UdhayaShan1 I have linked the AB3 tracing tutorial to the debugging guide. You'll need to update this PR accordingly.

# Conflicts:
#	tutorials/ab3TracingCode.md
@UdhayaShan1
Copy link
Author

Made changes accordingly.

@UdhayaShan1
Copy link
Author

image

Added deletion to the puml

@damithc
Copy link
Contributor

damithc commented Aug 25, 2024

@UdhayaShan1 can fix the conflict?

@damithc
Copy link
Contributor

damithc commented Sep 8, 2024

@UdhayaShan1 I was reviewing this PR for merging, and as I was reading it, I realized that while these extra details are useful, it may not be a good way to train students to do a similar tracing in future projects. That is because this version assumes there are detailed sequence diagrams already available for the reader to follow, but this is unrealistic (most projects don't even have proper architecture diagrams, let alone detailed sequence diagrams for each component). The best we can expect is, as the reader trace the code, s/he builds a sequence diagram along the way, to visualize the path being traced. For example, when explaining the execution path through the Logic component, the explanation should start with an almost empty sequence diagram and keep adding to it as the tracing progresses through the Logic component.
Because of this issue, I hesitate to merge this PR in the current state, although the work is still eligible for credit. If you have time, you can revise it to follow the 'build the SD as you go' approach. If you do, you can do a bit of it first as a PoC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AB3: Tracing Code] Add more headers to section instructions
4 participants