Android Tutorial [B4X] "Code Smells" - common mistakes and other tips

Status
Not open for further replies.
"Code smells" are common patterns that can indicate that there is a problem in the code. A problem doesn't mean that the code doesn't work, it might be that it will be difficult to maintain it or that there are more elegant ways to implement the same thing.
Remember that not everything is clear cut and there are exceptions for any rule.

I will update this list whenever I encounter such smell :)

  1. Initializing an object and then assigning a different object to the same variable:
    B4X:
    'bad
    Dim List1 As List
    List1.Initialize '<-- a new list was created here
    List1 = SomeOtherList '<--- previous list was replaced
    
    'good
    Dim List1 As List = SomeOtherList
  2. Deprecated methods - DoEvents, Msgbox: https://www.b4x.com/android/forum/t...cated-and-async-dialogs-msgbox.79578/#content
  3. Deprecated methods - Map.GetKeyAt / GetValueAt - these methods were added before the For Each loop was available. They are not cross platform and are not the correct way to work with maps.

    B4X:
    'bad
    For i = 0 To Map1.Size - 1
    Dim key As String = Map1.GetKeyAt(i)
    Dim value As String = Map1.GetValueAt(i)
    Next
    
    'good
    For Each key As String In Map1.Keys
    Dim value As String = Map1.Get(key)
    Next
  4. File.DirDefaultExternal - This is always a mistake. In most cases the correct folder should be XUI.DefaultFolder (=File.DirInternal). If you do need to use the external storage then use RuntimePermissions.GetSafeDirDefaultExternal.
    File.DirRootExternal - It will soon become inaccessible directly. If really needed then use ContentChooser or ExternalStorage.
  5. Not using parameterized queries:
    B4X:
    'very bad
    SQL.ExecNonQuery("INSERT INTO table1 VALUES ('" & EditText1.Text & "'") 'ugly, will break if there is an apostrophe in the text and vulnerable to SQL injections.
    
    'very good
    SQL.ExecNonQuery2("INSERT INTO table1 VALUES (?)", Array(EditText1.Text))
  6. Using Cursor instead of ResultSet - Cursor is a B4A only object. ResultSet is a bit simpler to use and is cross platform.
    B4X:
    'good
    Dim rs As ResultSet = SQL.ExecQuery2(...)
    Do While rs.NextRow
    ...
    Loop
    rs.Close
  7. Building the complete layout programmatically. This is especially a mistake in B4J and B4i because of the resize event and also if you want to build a cross platform solution. Layouts can be ported very easily.
  8. Repeating the code. There are many patterns to this one and all of them are bad.
    B4X:
    'bad
    If b = False Then
    Button1.Text = "disabled"
    Button2.Text = "disabled"
    Button3.Text = "disabled"
    Button1.Enabled = False
    Button2.Enabled = False
    Button3.Enabled = False
    Else
    Button1.Text = "enabled"
    Button2.Text = "enabled"
    Button3.Text = "enabled"
    Button1.Enabled = True
    Button2.Enabled = True
    Button3.Enabled = True
    End If
    B4X:
    'good
    For Each btn As Button In Array(Button1, Button2, Button3)
    btn.Enabled = b
    If b Then btn.Text = "enabled" Else btn.Text = "disable"
    Next
  9. Long strings without using smart strings:
    B4X:
    'bad
    Dim s As String = "This is the " & QUOTE & "first" & QUOTE & "line" & CRLF & _
    "and this is the second one. The time is " & DateTime.Time(DateTime.Now) & "."
    
    'good
    Dim s As String = $"This is the "first" line
    and this is the second one. The time is $Time{DateTime.Now}."$
    More information: https://www.b4x.com/android/forum/threads/50135/#content
  10. Using global variables when not needed:
    B4X:
    'bad
    Job.Initialize(Me, "") 'global variable
    ...
    
    'good
    Dim job As HttpJob
    job.Initialize(Me, "")
  11. Not using Wait For when possible. JobDone is a good example: [B4X] OkHttpUtils2 with Wait For
  12. Using code modules instead of classes. Code modules are very limited in B4A. In most cases you should use classes instead of code modules. A code module is a single instance of a class.
  13. Understanding booleans.
    B4X:
    'not elegant
    Dim result As Boolean = DoingSomethingThatReturnTrueOrFalse
    If result = True Then
    Return True
    Else
    Return False
    End If
    
    ' elegant
    Return DoingSomethingThatReturnTrueOrFalse
  14. Converting "random" bytes to strings. The only valid raw bytes that should be converted to a string, with BytesToString, are bytes that represent text. In all other cases it is a mistake to convert to string. Even if it seems to work it will later fail in other cases.
    If you think that it is more complicated to work with raw bytes then you are not familiar with the useful B4XBytesBuilder object: https://www.b4x.com/android/forum/threads/b4x-b4xcollections-more-collections.101071/#content
  15. Generating or parsing XML / JSON by hand. These formats are far from being trivial and with all kinds of edge cases that no one remembers.
    B4X:
    'bad
    Dim s As String = "{""version"":""" & Version & """,""colors"":[""red"",""green"",""blue""]}"
    
    'good
    Dim jg As JSONGenerator
    jg.Initialize(CreateMap("colors": Array("red", "green", "blue"), "version": Version))
    Log(jg.ToPrettyString(4))
Related thread: https://www.b4x.com/android/forum/threads/b4x-features-that-erel-recommends-to-avoid.133280/
That's it for now.
 
Last edited:

LucaMs

Expert
Licensed User
I'm not sure I understand this.
It is common for me to use GetKeyAt(Index) in conjunction with ComboBox.
the map keys are not related to the order of the combo list.
Agree with @sorex

Is it possible to rectify cbxEngineType_SelectedIndexChangeden in the code?
(Project Attached)
Thank you.
B4X:
Sub cbxEngineType_SelectedIndexChanged(Index As Int, Value As Object)
    lblKey.Text=enginesMap.GetKeyAt(Index)
End Sub
Few minutes ago 😄:
https://www.b4x.com/android/forum/threads/b4x-lmb4xcombobox-v-1-01.116767/post-729765
 

Jorge M A

Well-Known Member
Licensed User

It's very interesting @LucaMs Ms! I'm sure I'will try.
Thank you.

But I would like to see an answer from the perspective of Map, the deprecated methods .GetKeyAt, .GetValueAt, and of B4XOrderedMap.
 

OliverA

Expert
Licensed User
Is it possible to rectify cbxEngineType_SelectedIndexChangeden in the code?
(Project Attached)
Thank you.
Flip the map and use the Value portion of SelectedIndexChanged. See attached.
 

Attachments

  • MyComboMappedFlipped.zip
    2.6 KB · Views: 188

Jorge M A

Well-Known Member
Licensed User
Thank you, Erel.
For those interested:
B4X:
Sub ComboBox1_SelectedIndexChanged(Index As Int, Value As Object)
    label1.Text=orderedMap.Keys.Get(Index)
    label2.Text=orderedMap.Values.Get(Index)
End Sub
 

Erel

Administrator
Staff member
Licensed User
It is better to do it like this:
B4X:
Sub ComboBox1_SelectedIndexChanged(Index As Int, Value As Object)
    Dim Key As String = orderedMap.Keys.Get(Index)
    label1.Text = key
    label2.Text = orderedMap.Get(key)
End Sub
The "natural" way to get values is with the keys. Each call to OrderedMap.Values will create a new list for the values.
 

Jorge M A

Well-Known Member
Licensed User
I get "null" value with this.
My mistake. in my case the key is Int.
Works Fine!
B4X:
Sub ComboBox1_SelectedIndexChanged(Index As Int, Value As Object)
    Dim Key As Int = orderedMap.Keys.Get(Index)
    label1.Text = key
    label2.Text = orderedMap.Get(key)
End Sub
 

peacemaker

Expert
Licensed User
Point 12, of booleans - long non-elegant syntax is mostly for debug. How to debug without it (when result to be returned is not calculated yet) ?
 

techknight

Well-Known Member
Licensed User
I am a very verbose person when it comes to programming, so number 8 rubs me the wrong way. I tend to gravitate to the first statement as thats the method that I am familiar with, easier for me to understand, and the only way i will ever do it. IMHO. No right or wrong way there in my opinion.

BTW this is why I cant stand C or any of those other curlybrace languages.
 

Erel

Administrator
Staff member
Licensed User
I tend to gravitate to the first statement as thats the method that I am familiar with, easier for me to understand, and the only way i will ever do it.
Things evolve and mostly for good reasons. As I see it, developers should be open to learning new things. You spend a few minutes learning a new thing and you quickly get used to it.
 

LucaMs

Expert
Licensed User
I am a very verbose person when it comes to programming, so number 8 rubs me the wrong way. I tend to gravitate to the first statement as thats the method that I am familiar with, easier for me to understand, and the only way i will ever do it. IMHO. No right or wrong way there in my opinion.

BTW this is why I cant stand C or any of those other curlybrace languages.
I gave a Like for the first part of your comment, the "verbosity", on which I agree. Smart strings, however, do not make the code less readable, quite the opposite; they are really useful and very simple to use.
 

jnjft

Member
Licensed User
Having read this thread and reviewed some of my code I come to the conclusion that I'm in possession of a very insensible nose...

Thank you, I'm learning a lot from this!
 

Alexander Stolte

Expert
Licensed User
This is related to the way animations are implemented. It immediately sets the view's layout to the target layout and then animates it by animating the scale and translation properties. It makes it simple to use SetLayoutAnimated as the layout state is consistent.
That's a statement I wish I'd known sooner.
I wondered why with "SetLayoutAnimated" the label inside the layout is pulled so strangely during the animation.
 
Status
Not open for further replies.
Top