-
Notifications
You must be signed in to change notification settings - Fork 26
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
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # tutorials/ab3TracingCode.md
There was a problem hiding this 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).
tutorials/ab3TracingCode.md
Outdated
> 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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
Added remaining sections with edits reflected in PR description. |
There was a problem hiding this 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
## 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abp -> ecp ++: parse(1 n/Alice Yeoh) | |
abp -> ecp ++: parse("1 n/Alice Yeoh") |
Check other places as well.
tutorials/ab3TracingCode.md
Outdated
|
||
<annotate src="images/tracing/LogicSequenceDiagramWithMainWindow.png" width="900" alt="Sample Image"> | ||
<!-- Set Legend to both --> | ||
<a-point x="24%" y="16%" label="1"/> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
tutorials/ab3TracingCode.md
Outdated
### 2. LogicManager -> AddressBookParser | ||
|
||
<annotate src="images/tracing/LogicSequenceDiagramWithMainWindow.png" width="900" alt="Sample Image"> | ||
<!-- Set Legend to both --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- Set Legend to both --> |
Also update the alt
property
tutorials/ab3TracingCode.md
Outdated
<!-- 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"/> |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Thanks, changes made. Specifically,
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. |
@UdhayaShan1 I have linked the AB3 tracing tutorial to the debugging guide. You'll need to update this PR accordingly. |
# Conflicts: # tutorials/ab3TracingCode.md
Made changes accordingly. |
tutorials/images/tracing/LogicSequenceDiagramOnlyWithMainWindow.png
Outdated
Show resolved
Hide resolved
@UdhayaShan1 can fix the conflict? |
@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. |
EditCommand
EditCommand
under Tracing the execution pathFix #17