目录
1. Unused require statement
Line4 could be deleted.
2. Unused code _dateBound = true
Is this _dateBound really still needed? Is it previously introduced for lifecycle issue solution?
Comment by Jerry on 2015-01-23 17:00PM OK I am wrong.
3. Robustness of sap.cus.crm.lib.reuse.controls.Note.prototype.setModel
It is better not to make any assumptions that the consumer will call this method setModel as we expected.
Is there any possibility that all the internal reference like _noteTypeItemTemplate is still not initialized.
In this case, the method execution will cause javascript error.
4. This.getModel() VS model
Why not use model.getProperty directly? I have verified in debugger, there are exactly the same reference?
5. Naming convention
We can not judge the real content contained in these two variables at a first glance – A little confused about the meaning of noteType and _noteType.
Is it possible to add more information inline to variable _noteType, for example change _noteType
To _oNoteTypeSelect, so that the one who reads the source code could immediately know it is a reference for select control?
6. Not necessary to loop the whole data set every time to get language description
It is possible to have an inner buffer and every time we first try if the corresponding language description is already in inner buffer already:
Another example is setAggregation implementation by ui5 framework:
Get:
Set:
This logic is widely used in ui5 framework:
7. Unnecessary variable isDefault
Previously I assume there is some logic on isDefault like if ( XXX ) { isDefault = true } else { isDefault = false }
But actually it is not. Why not directly pass a true in line 51?
8. Define constant
Can we define some “constant” in init method to avoid resourceBundle call repeatedly?
For example, in init method,
Var EDIT_NOTE_DIALOG_TITLE = this.oResourceBundle.getText(“EDIT_NOTE_DIALOG_TITLE”);
9. Unnecessary variable
Use return new sap.ui.model.json.JSONModel(oData) instead. Comment by Jerry on 2015-01-23 17:15PM
Not necessary to change: 因为发现UI5的框架代码也这样用的：
10. Nested if
A little bit ugly, can we use the following one?
11. Is jQuery.proxy really necessary?
Haven’t yet measured the overhead of jQuery.proxy.
Some open source framework just use the following approach to avoid the additional jQuery.proxy call:
Also our own ui5 framework implementation:
doSo
12. Magic number
Can we use “constant” to avoid this magic number?
13. Better variable name?
After I go through the whole source code, I get to know that for note creation and edit case, we use the same
dialog instance, right? So _noteCreateDialog could be used both for create and Edit case?
In that case, it is better to rename _noteCreateDialog as _noteOperationDialog?
In the code below, it may confuse reader that the _getDialog can only construct Dialog for creation purpose.
14. Inconsistent naming convention
Sometimes we use prefix o to indicate the variable has object type, sometimes not, e.g noteCreateDialog.
15. Better method name
The below method assembles and finally return a model for given purpose – note creation and update.
Normally, we have two styles below:
So better name like getCalculatedModel4AddNoteDialog.
Comment by Jerry on 2015-01-23 17:14PM
Just see a similar usage as ours in JSONModel,js …. , so not necessary to change?
