chiageng / pe

0 stars 0 forks source link

DG missed out implementation of Add Transaction and View Transaction #19

Open chiageng opened 5 months ago

chiageng commented 5 months ago

Justification : These two features are main features of your app, there is a need for developer to refer it constantly in order to understand the architecture since from your code, these Transaction feature directly change the project structure majorly, hence is severity medium

Expected : Add uml diagram for these two features, or at least add transaction, so that the main structure able to explain to developer on what changes caused by this feature.

(it is a really good feature and i really think should add in to help developer to understand your project structure well, else i rely on reading code which is quite tough)

soc-pe-bot commented 5 months ago

Team's Response

Thank you for your feedback. However, we believe that we have sufficiently explained the implementations of adding and viewing transactions. These features are analogous to the add and find commands as described in the Model component of the Developer Guide (even much simpler implementations). The implementations of add and find commands are thoroughly elaborated in the Developer Guide. Additionally, it's important to minimize unnecessary overhead, and our documentation aims to provide just enough guidance to facilitate understanding and usage. While we appreciate the suggestion to include UML diagrams for these features, we believe that the clarity and simplicity of our code make such diagrams unnecessary.

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: These features are analogous to the add and find commands as described in the Model component of the Developer Guide

image.png

There is no explanation on how you implement Transaction and how Transaction is similar to Person. You should not assume developers "auto" know that both classes having similar implementation design without explaning in DG. There is only how Transaction interact with Model Component as explained in your DG.

even much simpler implementations should not be an asummption that developers will "auto" understand the architecture design. DG is meant to help developer to understand architecture design. This will cause huge confusion to developers without knowing what is the design of Transaction.

From your implementation in add transaction, there is a need to first check whether is this Person exist in your contact book, then you assign this Person id into one of the field of Transaction to keep association between Person.

Without explaining explicitly this design in DG, developers will not understand what is going on and hence causing more bugs if they misunderstand the design.

For example, they might consider that

  1. Add transaction will not have any relationship with person
  2. There is another separate association class named TransactionPerson to maintain relationship between Person and Transaction
  3. There is an array field in Person to store all the Transaction id for future access of all transaction from Person

All these different design will directly cause different code implementation and if developer mistaken as one of the wrong design, this app will fail to expand in future and creating more bugs.

Since this will directly impact developer, and developer always rely on DG to understand architecture of app, the severity should not be low as it is not rare case. In fact, severity should maintained as medium of higher as it might cause your app fail to expand in future if all developers unable to trace back the implementation design and draw a wrong design in future.


## :question: Issue severity Team chose [`severity.Low`] Originally [`severity.Medium`] - [x] I disagree **Reason for disagreement:** `These features are analogous to the add and find commands as described in the Model component of the Developer Guide ` ![image.png](https://raw.githubusercontent.com/chiageng/pe/main/files/0f67f336-1b0c-41ec-a36f-9db7037749c8.png) There is no explanation on how you implement Transaction and how Transaction is similar to Person. You should not assume developers "auto" know that both classes having similar implementation design without explaning in DG. There is only how Transaction interact with Model Component as explained in your DG. `even much simpler implementations` should not be an asummption that developers will "auto" understand the architecture design. DG is meant to help developer to understand architecture design. This will cause huge confusion to developers without knowing what is the design of Transaction. From your implementation in add transaction, there is a need to first check whether is this `Person` exist in your contact book, then you assign this `Person` id into one of the field of `Transaction` to keep association between `Person`. Without explaining explicitly this design in DG, developers will not understand what is going on and hence causing more bugs if they misunderstand the design. For example, they might consider that 1. Add transaction will not have any relationship with person 2. There is another separate association class named `TransactionPerson` to maintain relationship between `Person` and `Transaction` 3. There is an array field in `Person` to store all the `Transaction` id for future access of all transaction from `Person` All these different design will directly cause different code implementation and if developer mistaken as one of the wrong design, this app will fail to expand in future and creating more bugs. Since this will directly impact developer, and developer always rely on DG to understand architecture of app, the severity should not be low as it is not rare case. In fact, severity should maintained as medium of higher as it might cause your app fail to expand in future if all developers unable to trace back the implementation design and draw a wrong design in future.