Uploaded image for project: 'Titanium SDK/CLI'
  1. Titanium SDK/CLI
  2. TIMOB-8996

BlackBerry: Complete remaining issues from code review


    • Story Points:


      See pull request: #18

      1. void TiUIBase::setTiMappingProperties(const TI_PROPERTY* prop, int propertyCount)
      prop should be props since it's a pointer to an array of prop objects

      2. TiUIBase::setParametersFromObject(Local obj)

      Local propValue;
      + Handle propString;
      + TiObject* foundProp;

      3 last lines,
      It's cleaner to declare variables inside the smallest scope they are to be used in and also helps prevent referencing them outside of their scope. In this case the declarations should be inside the for

      3. int NativeContainerObject::initialize()
      add protection agains multiple invocations

      4. if((args.Length()!=2)||(!args[0]>IsString())||(!args[1]>IsFunction()))
      This line is definitely worth a comment. How come args[0] needs to be a string and args[1] needs to be a function? reminds me of perl :S

      5. blackberry/tibb/TiUIObject.cpp

      createXXXX_ methods
      out of 14 lines of code in the functions, only 1 really differs. It'd be nice to have a way of abstrating that to remove code duplication which would be a maintenance nightmare as i also forsee many many such functions being created for all the UI controls we need to implement

      Similarly the associtated classes look very bare and very much alike, is there actual value in having all these look alike classes? Maybe the actual question is can we avoid them?

      6. VALUE_MODIFY TiPropertyMapObject::onValueChange(Handle oldValue, Handle newValue)
      the 3 lines inside the the ifs are the same in all cases, they should be rolled into a helper function.

      7. we should review the logical separation of classes with regards to cascades awareness, separation into folders

      8. review error handling strategy

      • run it by appcelerator

      9. class TiEventContainer
      please add spaces around the = sign for consistency with the rest of the code.
The formatter doesn't enforce it here.

      There is at least another instance of that in TiEvent.h

      10. blackberry/tibb/TiUIBase.h
      description comment references TiCascadesApp when it should be TiUIBase

      11. embed class TiInternalEventListener inside TiV8EventContainer? To reenforce it's internalness




            • Assignee:
              dcampbell David Campbell
              dcampbell David Campbell
            • Watchers:
              0 Start watching this issue


              • Created:

                Backbone Issue Sync

                • Titanium SDK/CLI <> Titanium Mobile
                  Synced with:
                  Sync status:
                  Last received:
                  Last sent:

                  Git Integration