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

Erel

Administrator
Staff member
Licensed User
"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. 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))
  5. 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
  6. 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.
  7. 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
  8. 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
  9. Using global variables when not needed:
    B4X:
    'bad
    Job.Initialize(Me, "") 'global variable
    ...
    
    'good
    Dim job As HttpJob
    job.Initialize(Me, "")
  10. Not using Wait For when possible. JobDone is a good example: [B4X] OkHttpUtils2 with Wait For
  11. 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.
  12. Understanding booleans.
    B4X:
    'not elegant
    Dim result As Boolean = DoingSomethingThatReturnTrueOrFalse
    If result = True Then
        Return True
    Else
        Return False
    End If
    
    ' elegant
    Return DoingSomethingThatReturnTrueOrFalse
  13. 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
That's it for now...
 
Last edited:

Alexander Stolte

Expert
Licensed User
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.
you should also write that you can simply initialize the class in the starter service. I have used CodeModules in the past, because initializing the class scared me. I didn't come up with the idea to simply initialize the class in the starter service and then make it public. :)

12. Not using XUI, many things are easier with the xui library
 

Erel

Administrator
Staff member
Licensed User
12. Not using XUI, many things are easier with the xui library
I almost always use B4XView for the various views. It is simple, cross platform and also includes a few features not available elsewhere.
However at this point it is not a mistake to use the platform specific types.
 

Sandman

Well-Known Member
Licensed User
Would this be considered a code smell? (It is a big pet peeve of mine when I see an evaluation that return only the value that just was evaluated instead of simply returning it in the first place. Not sure how to explain better, perhaps easier to understand by looking at the code.)
B4X:
' bad
result = doingSomethingThatReturnTrueOrFalse
If result = True Then
    Return True
Else
    Return False
End If

' good
return doingSomethingThatReturnTrueOrFalse
 

incendio

Well-Known Member
Licensed User
Not using parameterized query
I'm using sql a lots, there was a time when using parameterized query on insert command gave an error, while using traditional method worked fine.

I will try to look that command.
 
Last edited by a moderator:

incendio

Well-Known Member
Licensed User
Not everything can be parameterized. The table name and column names cannot be parameterized, however it is quite rare that you need to "parameterize" these things.
It was parameterized a value, not table name or column names, will post the codes if find it.
 

Mahares

Well Known Member
Licensed User
What do you do if you are interested in a given key at at a given index, do you still iterate over all keys until you find it and then exit the loop, Or you cannot use the key index as the map order changes every time you access the map.
 
Last edited:

udg

Expert
Licensed User
Not so much time ago I planned to start a thread in the Wish area of the forum about Best (and eventually Bad) practices.
Having Erel just list best practices as of today (and keep it updated) in order to show newcomers (and old timers too) what should generally be done (i.e. valid 80/90% of times) in most of the recurring circumstances would have been of great help for anybody, IMHO.
Yes, most if not all the info is already published in the forum and we know that "the search engine works" is a truth.
But consider a single thread, closed to comments, where you find "how to" do correctly almost anything..
 

LucaMs

Expert
Licensed User
Not so much time ago I planned to start a thread in the Wish area of the forum about Best (and eventually Bad) practices.
Having Erel just list best practices as of today (and keep it updated) in order to show newcomers (and old timers too) what should generally be done (i.e. valid 80/90% of times) in most of the recurring circumstances would have been of great help for anybody, IMHO.
Yes, most if not all the info is already published in the forum and we know that "the search engine works" is a truth.
But consider a single thread, closed to comments, where you find "how to" do correctly almost anything..
"Best practices" and this "Code Smells" are different things, so I think that another new thread for "Best practices" would be useful.
 

Erel

Administrator
Staff member
Licensed User
What do you do if you are interested in a given key at at a given index, do you still iterate over all keys until you find it and then exit the loop, Or you cannot use the key index as the map order changes every time you access the map.
Map is not built for this task. You should use B4XOrderedMap from B4XCollections.
 

sorex

Expert
Licensed User
will .getKeyAt & .getValueAt remain?

in some cases you don't need to go through the entire loop/map but just pick a random item or the first/last one.

it can be done with the loop ofcourse by using exit/return but it looks weird to use a loop for that.
 

Erel

Administrator
Staff member
Licensed User
will .getKeyAt & .getValueAt remain?
Yes.
in some cases you don't need to go through the entire loop/map but just pick a random item or the first/last one.
See my answer in post #16. Note that the iterator needs to loop internally over the items. You just don't see it. Correct solution for such rare cases is to use B4XOrderedMap. It gives you the keys as a list.
 

Jorge M A

Well-Known Member
Licensed User
Correct solution for such rare cases is to use B4XOrderedMap.
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
in some cases you don't need to go through the entire loop/map but just pick a random item
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
 

Attachments

Top