This is a list of all comments for UI-DEN-1090-1.

Review Summary: No summary


----------------------------------------
File: sources/canbus/caninterface.cpp
Revision Comment by pmontazemi on 19 December 2019, 13:45 
https://devapps.diality.us/cru/UI-DEN-1090-1#c632

  Replace 10, 4, 100, etc. with LETTER_PARAMETERS centralized on
  top of code.

  Reply by Behrouz NematiPour on 19 December 2019, 13:56
  > This one is a formating of the output console and is only
  > used in this method numbers are just preferences for nicer
  > output.
  > No use out of this method.

    Reply by pmontazemi on 19 December 2019, 14:01
    > RESOLVED.

----------------------------------------
File: sources/maintimer.cpp
Revision Comment by pmontazemi on 19 December 2019, 13:52 
https://devapps.diality.us/cru/UI-DEN-1090-1#c642

  Inconsistent spacing. Purpose?

  Reply by Behrouz NematiPour on 19 December 2019, 14:14
  > It's not inconsistent any space has a meaning.
  > I align signal/slots to identify incorrect connections.

    Reply by pmontazemi on 19 December 2019, 14:25
    > RESOLVED.

----------------------------------------
File: sources/canbus/messagebuilder.cpp
Revision Comment by pmontazemi on 19 December 2019, 13:47 
https://devapps.diality.us/cru/UI-DEN-1090-1#c635

  Again, what is the reason for inconsistent spacing throughout
  the entire code base?

  Reply by Behrouz NematiPour on 19 December 2019, 14:25
  > Same here, These are aligned parameters as I mentioned to
  > better identify the code bugs.

  Reply by pmontazemi on 19 December 2019, 14:25
  > RESOLVED.

Revision Comment by pmontazemi on 19 December 2019, 13:47 
https://devapps.diality.us/cru/UI-DEN-1090-1#c636

  Comments take space between ; and //, also make comments
  consistent.

  Reply by Behrouz NematiPour on 19 December 2019, 14:20
  > Fixed

    Reply by pmontazemi on 19 December 2019, 14:22
    > RESOLVED.

Revision Comment by pmontazemi on 19 December 2019, 13:48 
https://devapps.diality.us/cru/UI-DEN-1090-1#c637

  What CONST is this 3? Replace with LETTER_CONST and centralize
  all of them on top of code.

  Reply by Behrouz NematiPour on 19 December 2019, 14:16
  > Fixed

    Reply by pmontazemi on 19 December 2019, 14:22
    > RESOLVED.

Revision Comment by pmontazemi on 19 December 2019, 13:48 
https://devapps.diality.us/cru/UI-DEN-1090-1#c638

  Same here.

  Reply by Behrouz NematiPour on 19 December 2019, 14:16
  > Fixed.

    Reply by pmontazemi on 19 December 2019, 14:23
    > RESOLVED.

Revision Comment by pmontazemi on 19 December 2019, 13:48 
https://devapps.diality.us/cru/UI-DEN-1090-1#c639

  Replace 2 with LETTER_CONST and centralize all of them on top
  of the code.

  Reply by Behrouz NematiPour on 19 December 2019, 14:15
  > fixed...

    Reply by pmontazemi on 19 December 2019, 14:23
    > RESOLVED.

Revision Comment by pmontazemi on 19 December 2019, 13:49 
https://devapps.diality.us/cru/UI-DEN-1090-1#c640

  Same here.

  Reply by Behrouz NematiPour on 19 December 2019, 14:15
  > fixed ...

    Reply by pmontazemi on 19 December 2019, 14:23
    > RESOLVED.

----------------------------------------
File: sources/canbus/messagebuilder.h
Revision Comment by pmontazemi on 19 December 2019, 13:49 
https://devapps.diality.us/cru/UI-DEN-1090-1#c641

  I love this!

  Reply by Behrouz NematiPour on 19 December 2019, 13:57
  > Thanks.
  > Since it's going to be as part of the sdd in doxygen I tried
  > to be descriptive.

    Reply by pmontazemi on 19 December 2019, 14:25
    > RESOLVED.

----------------------------------------
File: main.cpp
Revision Comment by Dara Navaei on 19 December 2019, 14:17 
https://devapps.diality.us/cru/UI-DEN-1090-1#c657

  Is this a magic number?

  Reply by Behrouz NematiPour on 19 December 2019, 14:21
  > This code is for test to be able to enable or disable
  > CanInterface , MessageDispatcher console out to be able to
  > debug without rebuild even on target.
  > And I mentioned at top of the code that it's a Test code for
  > debuging

    Reply by Dara Navaei on 19 December 2019, 14:40
    > RESOLVED

Revision Comment by Dara Navaei on 19 December 2019, 14:18 
https://devapps.diality.us/cru/UI-DEN-1090-1#c658

  Magic number?

  Reply by Behrouz NematiPour on 19 December 2019, 14:22
  > This code is for test to be able to enable or disable
  > CanInterface , MessageDispatcher console out to be able to
  > debug without rebuild even on target.
  > And I mentioned at top of the code that it's a Test code for
  > debuging

    Reply by Dara Navaei on 19 December 2019, 14:40
    > RESOLVED

----------------------------------------
File: sources/canbus/messageinterpreter.cpp
Revision Comment by pmontazemi on 30 December 2019, 08:23 
https://devapps.diality.us/cru/UI-DEN-1090-1#c716

  I would recommend putting the extra spaces after the += sign
  rather than between the variable and its property.

  Reply by Behrouz NematiPour on 30 December 2019, 10:23
  > that is one operator not two.
  > This has a meaning and cannot be separated !
  > If you separate them you get compile error ...

    Reply by pmontazemi on 30 December 2019, 10:26
    > What I meant is vData +=          mPWMDtCycle.value;
    > instead of    vData += mPWMDtCycle          .value;
    > 
    > such that the .value is always stuck to the member
    > variable. This looks really odd as it stands.

      Reply by Behrouz NematiPour on 30 December 2019, 10:29
      > - I aligned the m(s) so I'm sure that I'm using the
      > correct variables since I have variables with the same
      > name with v at the beginning most of the time.
      >  - Also it's really easy to copy/paste variables
      > vertically.
      >  - Also it shows me at first look that I used ".value"
      > otherwise it won't work.

        Reply by pmontazemi on 30 December 2019, 10:36
        > RESOLVED.

----------------------------------------
File: sources/canbus/frameinterface.cpp
Revision Comment by pmontazemi on 19 December 2019, 13:46 
https://devapps.diality.us/cru/UI-DEN-1090-1#c633

  Convert 3 to LETTER_CONST and centralize all consts on top of
  code.

  Reply by pmontazemi on 19 December 2019, 14:25
  > RESOLVED.

  Reply by Behrouz NematiPour on 19 December 2019, 14:26
  > Fixed.

----------------------------------------
File: sources/canbus/frameinterface.h
Revision Comment by pmontazemi on 19 December 2019, 13:47 
https://devapps.diality.us/cru/UI-DEN-1090-1#c634

  Why so much space?

  Reply by pmontazemi on 19 December 2019, 14:25
  > RESOLVED.

  Reply by Behrouz NematiPour on 19 December 2019, 14:26
  > Same here, These are aligned parameters as I mentioned to
  > better identify the code bugs.

----------------------------------------
File: unittests/tst_canbus.cpp
Revision Comment by pmontazemi on 19 December 2019, 13:52 
https://devapps.diality.us/cru/UI-DEN-1090-1#c643

  Purpose of extra spacing?

  Reply by Behrouz NematiPour on 19 December 2019, 14:11
  > These are connection which I align to with the other once to
  > make sure I'm connecting ti the correct Signal/slot.
  > It helps me to identify the problem very quick.

    Reply by pmontazemi on 19 December 2019, 14:24
    > RESOLVED.

Revision Comment by pmontazemi on 19 December 2019, 13:53 
https://devapps.diality.us/cru/UI-DEN-1090-1#c644

  Make this a LETTER_CONSTANT and centralize all of them on top
  of code.

  Reply by Behrouz NematiPour on 19 December 2019, 14:13
  > Some Constants are one time use and doesn't add any value to
  > the code to collect them in one place.

    Reply by pmontazemi on 19 December 2019, 14:24
    > RESOLVED.

Revision Comment by pmontazemi on 19 December 2019, 13:53 
https://devapps.diality.us/cru/UI-DEN-1090-1#c645

  Let's discuss as a Team whether it makes sense to centralize
  these as LETTER_CONST in one place or leave it as is.

  Reply by Behrouz NematiPour on 19 December 2019, 14:12
  > Some Constants are one time use and doesn't add any value to
  > the code to collect them in one place.
  > And also since it is the unit test class the constants have
  > been tweaked and modified to get errors to test the code.
  > So even if it's look like the other one it has some slight
  > changes.

    Reply by pmontazemi on 19 December 2019, 14:24
    > RESOLVED.

----------------------------------------
File: sources/utility/types.cpp
Revision Comment by pmontazemi on 30 December 2019, 08:25 
https://devapps.diality.us/cru/UI-DEN-1090-1#c718

  Isn't there a library or a method that already does this?

  Reply by Behrouz NematiPour on 30 December 2019, 10:14
  > I agree that it's a little bit redundant but after facing an
  > error I found a subject and this approach was recommended and
  > it worked for me.
  > This happens since I need to convert array of bytes to float
  > value and then to variant which GUI understands and during
  > this conversion some values get more that expected values
  > after decimal point.

    Reply by pmontazemi on 30 December 2019, 10:25
    > RESOLVED.


---

ID: UI-DEN-1090-1 https://devapps.diality.us/cru/UI-DEN-1090-1

Title: UI-DEN-1090_UI Message Handler / Error Handler
Statement of Objectives:


State: Closed

Summary: 

Author: Behrouz NematiPour
Moderator: Behrouz NematiPour
Reviewers: (5 active, 3 completed*)
     Dara Navaei   (*)
     pmontazemi   (*)
     lbaloa   (*)
     Sean Nash
     vduong
     Tiffany Mejia
     Michael Garthwaite
     msuleiman