This journal is part of series on Software Engineering,
-
Bad coding practices (this article).
This journal collect the bad coding practices that I have found during my journey maintaining legacy software.
The term "coding" in this context does not only means "a line of code" or "programming language", more on "software engineering" in general.
Storing time not in UTC
Let say your server store the time in UTC+0900 timezone and you have multiple clients on different timezone, one in UTC-1000 and another one in UTC+0700. Which time should the client display? Their local time or server time?
If your answer is "its depends", you may be correct, the software in your case may require that all the clients reference the central timezone which is UTC+9. And to do this, the application on the client side needs to convert the time twice; first converting to UTC and then converting it to local time zone.
But, 99% of the time, it should displayed in the user time zone.
If as a user, I read and translate time manually every time I see date time record and ask "what time is this?" in my head, that’s indicate there is some thing wrong in the application. Imagine this happened when we are handling hundreds if not thousands of records.
The rule of thumb is store time in UTC and display time in local time zones.
Logic in the client
Nowadays, we usually split the front-end (the code for user interface) and back-end (the code that process input from front-end and read/write to the database).
The bad coding practices is when the front-end try to process the action before sending it to backend by himself based on certain condition.
For example, given a list of record, one of them should be set to default,
in this case id:100.
[
{id:1, is_default: false}
{id:2, is_default: false}
{id:3, is_default: false}
...
{id:100, is_default: true}
{id:101, is_default: false}
]
When user want to changes the "is_default" to 3, the front-end then check
the record one by one.
If its found the existing record (id:100) then it will call the API on the
backend to set id:100 is_default to false, and set the id:3 is_default
to true.
The problem is if the front-end receive only partial of data, due to paging,
there is a chance that existing is_default record is not on the same page as
new one.
This cause two records, id:3 and id:100, have is_default to true.
The good practices is never process any logic on the front-end. Front-end responsibilities should be displaying data only, and receive and forward commands from user to the backend. Any logic should be handled by backend.
The solution of above problem is quite simple on the backend,
FUNC setIsDefault(newDefault)
BEGIN
oldDefault := getIsDefault()
IF oldDefault IS FOUND THEN
oldDefault.setIsDefault(false)
END
newDefault.setIsDefault(true)
END
Grouping same class file into one directory
This is a pattern that I always see where developers group two or more files that are not related to each other but have the same "class" into one directory. For example, grouping all controllers into one directory called "controllers", grouping all models into one directory called "models", and so on.
├── controllers │ ├── feature-A.ctrl │ ├── feature-B.ctrl │ ├── feature-C.ctrl │ ├── feature-D.ctrl ... ├── models │ ├── feature-A.model │ ├── feature-B.model │ ├── feature-C.model │ ├── feature-D.model ... ├── views │ ├── feature-A.view │ ├── feature-B.view │ ├── feature-C.view │ ├── feature-D.view ...
This pattern making hard to navigate the source code. When you open the view code, you need to jump to other directory to view what the view trigger, and then jump again to another directory to lookup what the model of data that controller manages.
The good practices is by coupling them by feature,
├── feature-A
│ ├── feature-A.ctrl
│ ├── feature-A.model
│ ├── feature-A.view
...
├── feature-B
│ ├── feature-B.ctrl
│ ├── feature-B.model
│ ├── feature-B.view
│
├── feature-C
│ ├── feature-C.ctrl
│ ├── feature-C.model
│ ├── feature-C.view
...
In this way, the scope that directory provides is limited by feature. We can also make dependencies between features also clear. For example, we can say that feature-C exist only when feature-B is enabled or depends on feature-B to be functional.
One component many functions
There is this form where a record can be created or updated. The form tied to a controller (or a service) that do both of the thing. In the view, we use a condition, if mode is "create" we display the "Create" button; if mode is "update" we display the "Update" button. Both of this buttons call different function but in the same controller.
The bad practices is when mixing two different functionalities forced into
one component (one controller and one view).
The controller and view littered with if-updateMode-else or
if-createMode-else conditions, which makes the code hard to read and
changes.
The good practice to solve this kind of problem is by creating two separate pages with shared form component and two different controllers. The mode and functionality then passed to view component as parameters. For example, on the page that create new book, the form can be instantiated by,
<my-form mode=create on-submit=doCreate>
While on page that update the book, the form is instantiated with
<my-form mode=update on-submit=doUpdate>
In the form, we can still have if-else to disable or hide some fields
or information, but at least this only happened in the view.
Logic in view
Nothing smell like bad code than this.
In Model-View-Controller, the view is the layer that display the data (model) and forward command to controller.
In any design pattern, the view should not contains logic. By logic, I means the lines of code contains something even as simple as comparison or ternary operation.
<component hidden="{{ isMode == 'edit' }}">
The good practices by using and initializing variable inside the controller and reference that in view as variable only.
// In controller. this.isHidden = (isMode == 'edit');
// In view. <component hidden="isHidden">
Unnecessary function split
In college, we have being teach that we should split larger function into smaller functions. The next question, is when to split it? and how to split it?
The bad practice is when the function body contain less than 10 lines (or on range 20-30 depends on your flavour) AND only called once AND does not affect the flow of the caller or program.
For example,
FUNCTION doX
...
doY()
...
FUNCTION doY
stmt1
stmt2
You can see that function "doY" is called from "doX" and it does not affect the flow or have any purpose except that it’s being "splitted".
Splitting "doY" because it changes the flow is little bit make sense, for example,
FUNC doX
...
IF doY(); THEN
...
but still, if its only couple of lines there is no harm on writing it on the parent function. In fact, it help the reader to read the code it without jumping to another, unnecessary context.
The worst part of this practice that I found is the function "doY" is on different file called "common" or "util" AND no one, I repeat, no other function used it except the "doX".
On web application
Using right click to show menu
(Note: this is fall into bad user experience, not coding).
In non-web application, using right click to show additional menus make senses because there is no default menu or event will show by OS.
In web application, right click menu is belong to browser, not application.
The problem is when some one new to your application, no one can guess that certain actions can be done by right click the item, because intuitively right click means show browser actions.
The good practices is by adding a little icon "…" on each item that can be right-clicked, so user can see and click it.
Deriving mode from URL path
Given the following URL for editing a record: "/book/:id" and URL for creating a record "/book/create", a single page is created using the same view and controller. The controller check that,
-
if "id" exist then the current context of the page is in update mode and the view has an "Update" button;
-
If the "id" did not exist then the context of the page is in create mode, and the view has a "Submit" button.
The bad practice is when using the same URL path or levels for two different purpose, one for create and the other one for update:
-
"/book/create", for creating new book, on path with 2 levels.
-
"/book/:id", for updating book, also on path with 2 levels.
The solution is quite simple, add a verb after book path for update like "/book/edit/:id", so the add and edit are handled by different page:
-
"/book/create" for creating new book, and
-
"/book/edit/…" for updating book record.
On testing
Verifying test results from the same sources
You have an API that read data from database. You seed the database manually, from predefined records. You call the API to get the results and compare them to test that the API behave as you expected.
The bad practices is when you use the same sources to compare the expected and test results, in this case both from the database. When verifying data, the sources must be different.
seeds := [recordA, recordB]
FOR EACH item in seeds; DO
INSERT item INTO DATABASE;
DONE
testResult := callApiToBeTested()
expectedResult := queryTheDatabaseDirectly()
assert testResult == expectedResult // BAD!!!
If you do this there is no different between test and expected, especially if you use the same function to read the database.
SEEDS --> DATABASE --> API --> TEST RESULT
|
+-------> READ --> EXPECTED RESULT (X)
What you should do is comparing them with predefined records from seeds. This is not only to test that the data being inserted is correct both to verify that we comparing two data from different sources.
seeds := [recordA, recordB]
FOR EACH item in seeds; DO
INSERT item INTO DATABASE;
DONE
testResult := callApiToBeTested()
assert testResult == seeds // GOOD!