Titanium JIRA Archive
Titanium SDK/CLI (TIMOB)

[TIMOB-8996] BlackBerry: Complete remaining issues from code review

GitHub Issuen/a
TypeStory
PriorityHigh
StatusClosed
ResolutionFixed
Resolution Date2012-06-29T08:33:24.000+0000
Affected Version/sn/a
Fix Version/sRelease 2.1.0, Sprint 2012-08 BB
ComponentsBlackBerry
Labelsn/a
ReporterDavid Campbell
AssigneeDavid Campbell
Created2012-05-04T12:12:27.000+0000
Updated2017-03-02T21:41:00.000+0000

Description

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

Comments

  1. Jean-Philippe Lemieux 2012-05-04

    Wasn't it PR #35?
  2. David Campbell 2012-05-25

    Pull request: https://github.com/Macadamian/titanium_mobile/pull/60
  3. Lee Morris 2017-03-02

    Closing ticket as Blackberry is no longer supported.

JSON Source