This is a list of all comments for UI-DEN-4690-2-1. Review Summary: No summary General Comment by plucia on 30 September 2020, 11:25 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c4992 This review has so many files and is quite monumental. It would be much easier to review if there were fewer changes. What are the list of features that are covered in this review? It seems like it's not just doxygenization Reply by Behrouz NematiPour on 07 October 2020, 22:15 > It is all documentations and Doxygenization. > If you see otherwise is because of the some file name change > and the code review take it as a new file and mark all the > file content as to be reviewed. > Let me know if otherwise. Reply by plucia on 14 October 2020, 10:43 > RESOLVED ---------------------------------------- File: sources/abstract/singleton.h Revision Comment by plucia on 30 September 2020, 11:24 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c4991 There needs to be documentation for this class Reply by Behrouz NematiPour on 30 September 2020, 11:34 > This file has been renamed to singletone.h._ and has been > removed by Peman request. > So this file doesn't exist anymore. Reply by plucia on 30 September 2020, 11:44 > RESOLVED ---------------------------------------- File: sources/applicationcontroller.cpp Revision Comment by pmontazemi on 30 September 2020, 15:18 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c5033 How come these files have been renamed but they have not been deleted to show in red? Reply by Behrouz NematiPour on 05 October 2020, 10:58 > Good point, but I don't know. > Seems like it's the way Crucible prefers to show the files. > It has been removed and even the git log shows that. > This commit is the last commit no the ApplicationController > and there is no applicationcontroler on that commit anymore. > http://192.168.10.132:8060/changelog/application/?cs=bb74da05f81b82dad3ec844c1feb1135b949f1c2 Reply by pmontazemi on 06 October 2020, 09:43 > RESOLVED. ---------------------------------------- File: sources/canbus/caninterface.cpp Revision Comment by pmontazemi on 30 September 2020, 15:13 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c5024 How come the renaming was done on all these files but the older files are still there and not marked as removed in red? Reply by Behrouz NematiPour on 05 October 2020, 11:09 > Good point, I don't know. > This is the way Crucible shows them. Reply by pmontazemi on 06 October 2020, 09:44 > RESOLVED. ---------------------------------------- File: sources/gui/guicontroller.cpp Revision Comment by pmontazemi on 30 September 2020, 15:15 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c5027 How come the renaming of files was done but the old files are not in red? Reply by Behrouz NematiPour on 05 October 2020, 11:09 > Good point, I don't know. > This is the way Crucible shows them. Reply by pmontazemi on 06 October 2020, 09:44 > RESOLVED. ---------------------------------------- File: sources/gui/qml/pages/ManagerHome.qml Revision Comment by plucia on 30 September 2020, 15:43 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c5045 Which of these classes will need to be in the production code? {code} VDGROPump VDGDrainPump VDGPressures VDGReservoir VDGHeaters VDGLoadCellReadings VDGTemperatures VDGValveStates VDGOperationMode VTreatmentBloodFlow VTreatmentDialysateFlow VTreatmentUltrafiltration VTreatmentPressureOcclusion {code} Are any only for in-development testing / convenience for systems? Reply by Behrouz NematiPour on 05 October 2020, 10:54 > "*** Off the subject ***" > AGAIN, > I don't understand what is the relevance of the comment to > the Story and which part of the code this comment is > referring to? Reply by plucia on 14 October 2020, 10:42 > Fair, not related to story. > > RESOLVED. ---------------------------------------- File: sources/storage/filehandler.cpp Revision Comment by pmontazemi on 30 September 2020, 15:16 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c5028 How come renaming of files was done but the old files are not shown in red as deleted/ Reply by Behrouz NematiPour on 05 October 2020, 11:08 > Good point, I don't know. > This is the way Crucible shows them. Reply by pmontazemi on 06 October 2020, 09:44 > RESOLVED. ---------------------------------------- File: sources/utility/crc.cpp Revision Comment by pmontazemi on 30 September 2020, 15:16 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c5029 Should all files under utility be renamed as well? Reply by Behrouz NematiPour on 05 October 2020, 11:05 > I kept these files noncapital since they have short one word > only files and no need to be PascalCase. Reply by pmontazemi on 06 October 2020, 09:43 > RESOLVED. ---------------------------------------- File: sources/view/valarmstatus.cpp Revision Comment by pmontazemi on 30 September 2020, 15:17 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c5031 Should these files be renamed following our convention? Reply by Behrouz NematiPour on 05 October 2020, 11:03 > This file and all the view file has been renamed to CamelCase > but seems Crucible still insist to get a review on them. Reply by pmontazemi on 06 October 2020, 09:44 > RESOLVED. ---------------------------------------- File: simulator/run.py Revision Comment by plucia on 30 September 2020, 11:00 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c4983 It's preferred to not use a global variable unless absolutely necessary Reply by Behrouz NematiPour on 30 September 2020, 11:47 > Totaly agree but I didn't use classes at the beginning. > When I improve the simulator will consider using class and > remove the globals. Reply by plucia on 30 September 2020, 14:22 > Okay, please provide a link that Jira case where you plan > to improve the simulator Reply by Behrouz NematiPour on 30 September 2020, 14:32 > I have no plan. > If it requires any new feature/improvement etc then I'll > start using classes next time I'm getting into that code. Reply by plucia on 30 September 2020, 15:48 > Better to fix it now then never Reply by Behrouz NematiPour on 30 September 2020, 16:05 > "*** Off the subject ***" > I don't have any plans for that. > It was an effort to feel the gap of not having the > corresponding FW code at that moment. > If it happens again will consider improving the code > otherwise we need to consolidate with our manager for > priorities/planning. Reply by plucia on 14 October 2020, 10:44 > RESOLVED. > > Fair, not related to story Revision Comment by plucia on 30 September 2020, 15:33 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c5043 Please indicate the return type as "None" if a function doesn't return anything Reply by Behrouz NematiPour on 13 October 2020, 18:07 > did Reply by plucia on 14 October 2020, 10:44 > RESOLVED ---------------------------------------- File: tst_Treatment_Adjustment_Ultrafiltration/test.py Revision Comment by plucia on 30 September 2020, 15:35 https://devapps.diality.us/cru/UI-DEN-4690-2-1#c5044 The functions are missing docstrings. Function descriptions and parameter types and descriptions, as well as the return type are needed Reply by Behrouz NematiPour on 06 October 2020, 12:40 > This is the test code. > Not the API, not the production code. > And not even being used in documentation. > I don't think we need to put comments for these functions as > long as the name is self-descriptive within context. > As I never asked you to comment on your test functions. Reply by plucia on 14 October 2020, 10:43 > RESOLVED. --- ID: UI-DEN-4690-2-1 https://devapps.diality.us/cru/UI-DEN-4690-2-1 Title: UI-DEN-4690-2_Doxygenization Statement of Objectives: State: Closed Summary: Author: Behrouz NematiPour Moderator: Behrouz NematiPour Reviewers: (0 active, 2 completed*) plucia (*) pmontazemi (*)