[TIMOB-8996] BlackBerry: Complete remaining issues from code review
GitHub Issue | n/a |
---|---|
Type | Story |
Priority | High |
Status | Closed |
Resolution | Fixed |
Resolution Date | 2012-06-29T08:33:24.000+0000 |
Affected Version/s | n/a |
Fix Version/s | Release 2.1.0, Sprint 2012-08 BB |
Components | BlackBerry |
Labels | n/a |
Reporter | David Campbell |
Assignee | David Campbell |
Created | 2012-05-04T12:12:27.000+0000 |
Updated | 2017-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
Wasn't it PR #35?
Pull request: https://github.com/Macadamian/titanium_mobile/pull/60
Closing ticket as Blackberry is no longer supported.