目录

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:

If ( oLanguageDescriptionSet[languageCode] ) {
return oLanguageDescriptionSet[languageCode];
}
else {
// old logic
oLanguageDescriptionSet[languageCode] = <fetched description via loop>
}

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?

If( changeType === “” ) && ( !this._hasNote(noteType, code)) {
       Languages.push(instance);
}

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:

Void doSomething() {  
      // do something 
.. } 

<return type> getSomething()  { 
      // prepare something 
 Return something; 
} 

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?

要获取更多Jerry的原创文章，请关注公众号"汪子熙":