Software Engineering Coding - Bad Practices


Google+ Facebook Twitter LinkedIn Dzone Reddit Digg Blogger Hacker News Addthis

By Markus Sprunck; Revision: 1.0; Status: final; Last Content Change: Nov 29, 2012;

The following design and coding anti-patterns are explained with samples of a real software I developed in Turbo Pascal about 20 years ago. Last week, I rewrote it to a pure Java application and published the result in the article An Example of Finite Element Method Simulation in Pure Java and HTML5 with Paper.js Library

My first idea was - that this would be a job for an afternoon. Actually, I needed significantly more time. The reasons for the extra effort have been some poor coding practices I have had as young developer. Although the old Pascal software worked properly and created the expected results, the maintainability was quite poor. After such a long time - even you own code could have been written by a stranger. Maybe, I needed even more time than for writing everything new from scratch, but I leaned some lessons I'd like to share with you. 

The approach of rewriting was straightforward:
  • just translated the old Pascal code into Java syntax, 
  • write some minimal JUnit test cases and 
  • do the refactoring and enhance the unit tests (until the code its good enough).

No Unit Tests

My old program had not one line of test code. This is not a nice situation, if you like to change the old source code. But I never intended to change the old source code. So, "Why do I need unit test code to rewrite a software in another language?" The answer is easy, automated unit test cases also serve as a kind of documentation and/or specification. Unit test describe the expected behavior in a better way than most of the written documentation.

As workaround, I had to run the old software in an emulated environment and recorded the actual results as expected behavior for the rewritten code. This experience encouraged me again to say: "Always write some basic unit test, even if your software works well. Some day you will be happy about your unit tests."

Don't forget that JUnit was born in late year 1997. My first contact with test automation was about at that time. There are still too much Never-Write-Test-Code-Developers working.   

Missing Comments

I'm still not a friend of exhaustive comments (see also Top 5 Reasons for Not Using JavaDoc in the Next Project), but almost no comments are really a bad idea. 

You may be lazy with writing comments, but in that case the code should speak to you in a meaningful way. This means that all classes, methods and variables need speaking names. Sometimes there is still the need for additional comments.

No Self-Explanatory Variable Names

A good sample are the following variable names. When I started to rewrite the old source code, I didn't know what the difference between g, ge, Ae or EE was. Maybe it have been clear during writing the code, but after some years these
name make absolutely no sense.

// Pascal - Sample Code #1:
    PRIVATE                    
                    ny                              : real                  ; 
                    E                               : real                  ; 
                    g                               : g_type                ; 
                    ge                              : ge_array_type         ; 
                    Ae                              : flaechen_array_type   ; 
                    KB                              : K_Bandmatrix_type     ; 
                    EE                              : e_array_type          ; 

In this case even the data typed didn't help, as you may notice from the names.

Optimization Without Comment what Happens

Just have a look at the following code sample. If you are able to understand what is implemented here within a minute you can be proud. At the end of the code sample you will find a short explanation and the same in a not optimized solution.

// Pascal - Sample Code #5: Obfuscation by optimization happens quite often.
PROCEDURE Ke_matrix_berechnen(VAR K_e_: Ke_Matrix_type; D: D_Matrix_type; B: B_Matrix_type; 
                                  Dicke_1, flaeche_1 : real); 

  TYPE  Hilfs_Matrix_type     = ARRAY [1..3,1..6] OF real; 
  VAR   h          :   Hilfs_Matrix_type; 
  BEGIN
    h[1,1] :=  Flaeche_1*Dicke_1*(  B[1,1]*D[1,1]+B[1,2]*D[2,1]+B[1,3]*D[3,1]); 
    h[2,1] :=  Flaeche_1*Dicke_1*(  B[1,1]*D[1,2]+B[1,2]*D[2,2]+B[1,3]*D[3,2]); 
    h[3,1] :=  Flaeche_1*Dicke_1*(  B[1,1]*D[1,3]+B[1,2]*D[2,3]+B[1,3]*D[3,3]); 
    h[1,2] :=  Flaeche_1*Dicke_1*(  B[2,1]*D[1,1]+B[2,2]*D[2,1]+B[2,3]*D[3,1]); 
    h[2,2] :=  Flaeche_1*Dicke_1*(  B[2,1]*D[1,2]+B[2,2]*D[2,2]+B[2,3]*D[3,2]); 
    h[3,2] :=  Flaeche_1*Dicke_1*(  B[2,1]*D[1,3]+B[2,2]*D[2,3]+B[2,3]*D[3,3]); 
    h[1,3] :=  Flaeche_1*Dicke_1*(  B[3,1]*D[1,1]+B[3,2]*D[2,1]+B[3,3]*D[3,1]); 
    h[2,3] :=  Flaeche_1*Dicke_1*(  B[3,1]*D[1,2]+B[3,2]*D[2,2]+B[3,3]*D[3,2]); 
    h[3,3] :=  Flaeche_1*Dicke_1*(  B[3,1]*D[1,3]+B[3,2]*D[2,3]+B[3,3]*D[3,3]); 
    h[1,4] :=  Flaeche_1*Dicke_1*(  B[4,1]*D[1,1]+B[4,2]*D[2,1]+B[4,3]*D[3,1]); 
    h[2,4] :=  Flaeche_1*Dicke_1*(  B[4,1]*D[1,2]+B[4,2]*D[2,2]+B[4,3]*D[3,2]); 
    h[3,4] :=  Flaeche_1*Dicke_1*(  B[4,1]*D[1,3]+B[4,2]*D[2,3]+B[4,3]*D[3,3]); 
    h[1,5] :=  Flaeche_1*Dicke_1*(  B[5,1]*D[1,1]+B[5,2]*D[2,1]+B[5,3]*D[3,1]); 
    h[2,5] :=  Flaeche_1*Dicke_1*(  B[5,1]*D[1,2]+B[5,2]*D[2,2]+B[5,3]*D[3,2]); 
    h[3,5] :=  Flaeche_1*Dicke_1*(  B[5,1]*D[1,3]+B[5,2]*D[2,3]+B[5,3]*D[3,3]); 
    h[1,6] :=  Flaeche_1*Dicke_1*(  B[6,1]*D[1,1]+B[6,2]*D[2,1]+B[6,3]*D[3,1]); 
    h[2,6] :=  Flaeche_1*Dicke_1*(  B[6,1]*D[1,2]+B[6,2]*D[2,2]+B[6,3]*D[3,2]); 
    h[3,6] :=  Flaeche_1*Dicke_1*(  B[6,1]*D[1,3]+B[6,2]*D[2,3]+B[6,3]*D[3,3]); 
    
    K_e_[1,1] :=   h[1,1]*B[1,1]+h[2,1]*B[1,2]+h[3,1]*B[1,3]; 
    K_e_[2,1] :=   h[1,1]*B[2,1]+h[2,1]*B[2,2]+h[3,1]*B[2,3]; 
    K_e_[3,1] :=   h[1,1]*B[3,1]+h[2,1]*B[3,2]+h[3,1]*B[3,3]; 
    K_e_[4,1] :=   h[1,1]*B[4,1]+h[2,1]*B[4,2]+h[3,1]*B[4,3]; 
    K_e_[5,1] :=   h[1,1]*B[5,1]+h[2,1]*B[5,2]+h[3,1]*B[5,3]; 
    K_e_[6,1] :=   h[1,1]*B[6,1]+h[2,1]*B[6,2]+h[3,1]*B[6,3]; 
    
    K_e_[1,2] :=   h[1,2]*B[1,1]+h[2,2]*B[1,2]+h[3,2]*B[1,3]; 
    K_e_[2,2] :=   h[1,2]*B[2,1]+h[2,2]*B[2,2]+h[3,2]*B[2,3]; 
    K_e_[3,2] :=   h[1,2]*B[3,1]+h[2,2]*B[3,2]+h[3,2]*B[3,3]; 
    K_e_[4,2] :=   h[1,2]*B[4,1]+h[2,2]*B[4,2]+h[3,2]*B[4,3]; 
    K_e_[5,2] :=   h[1,2]*B[5,1]+h[2,2]*B[5,2]+h[3,2]*B[5,3]; 
    K_e_[6,2] :=   h[1,2]*B[6,1]+h[2,2]*B[6,2]+h[3,2]*B[6,3]; 
    
    K_e_[1,3] :=   h[1,3]*B[1,1]+h[2,3]*B[1,2]+h[3,3]*B[1,3]; 
    K_e_[2,3] :=   h[1,3]*B[2,1]+h[2,3]*B[2,2]+h[3,3]*B[2,3]; 
    K_e_[3,3] :=   h[1,3]*B[3,1]+h[2,3]*B[3,2]+h[3,3]*B[3,3]; 
    K_e_[4,3] :=   h[1,3]*B[4,1]+h[2,3]*B[4,2]+h[3,3]*B[4,3]; 
    K_e_[5,3] :=   h[1,3]*B[5,1]+h[2,3]*B[5,2]+h[3,3]*B[5,3]; 
    K_e_[6,3] :=   h[1,3]*B[6,1]+h[2,3]*B[6,2]+h[3,3]*B[6,3]; 
    
    K_e_[1,4] :=   h[1,4]*B[1,1]+h[2,4]*B[1,2]+h[3,4]*B[1,3]; 
    K_e_[2,4] :=   h[1,4]*B[2,1]+h[2,4]*B[2,2]+h[3,4]*B[2,3]; 
    K_e_[3,4] :=   h[1,4]*B[3,1]+h[2,4]*B[3,2]+h[3,4]*B[3,3]; 
    K_e_[4,4] :=   h[1,4]*B[4,1]+h[2,4]*B[4,2]+h[3,4]*B[4,3]; 
    K_e_[5,4] :=   h[1,4]*B[5,1]+h[2,4]*B[5,2]+h[3,4]*B[5,3]; 
    K_e_[6,4] :=   h[1,4]*B[6,1]+h[2,4]*B[6,2]+h[3,4]*B[6,3]; 
    
    K_e_[1,5] :=   h[1,5]*B[1,1]+h[2,5]*B[1,2]+h[3,5]*B[1,3]; 
    K_e_[2,5] :=   h[1,5]*B[2,1]+h[2,5]*B[2,2]+h[3,5]*B[2,3]; 
    K_e_[3,5] :=   h[1,5]*B[3,1]+h[2,5]*B[3,2]+h[3,5]*B[3,3]; 
    K_e_[4,5] :=   h[1,5]*B[4,1]+h[2,5]*B[4,2]+h[3,5]*B[4,3]; 
    K_e_[5,5] :=   h[1,5]*B[5,1]+h[2,5]*B[5,2]+h[3,5]*B[5,3]; 
    K_e_[6,5] :=   h[1,5]*B[6,1]+h[2,5]*B[6,2]+h[3,5]*B[6,3]; 
    
    K_e_[1,6] :=   h[1,6]*B[1,1]+h[2,6]*B[1,2]+h[3,6]*B[1,3]; 
    K_e_[2,6] :=   h[1,6]*B[2,1]+h[2,6]*B[2,2]+h[3,6]*B[2,3]; 
    K_e_[3,6] :=   h[1,6]*B[3,1]+h[2,6]*B[3,2]+h[3,6]*B[3,3]; 
    K_e_[4,6] :=   h[1,6]*B[4,1]+h[2,6]*B[4,2]+h[3,6]*B[4,3]; 
    K_e_[5,6] :=   h[1,6]*B[5,1]+h[2,6]*B[5,2]+h[3,6]*B[5,3]; 
    K_e_[6,6] :=   h[1,6]*B[6,1]+h[2,6]*B[6,2]+h[3,6]*B[6,3]; 
  END; 
The logic of the code up there is: 

       a XY X      

where X and Y are two matrices and a (equal to area * thickness) is a scalar value. The same in Java with a simple helper class for matrix operations looks like this:

// Java - Sample Code #6: A higher level of abstraction may improve the readability 

return X.times(Y).times(X.transpose()).mult(area*thickness);

Notice, the excessive unfolding of loops was a common practice 20 years ago. Today it is not a good idea. The modern compilers and in Java case the JIT compiler are more powerful than most manual optimizations. In some cases it is even slower to have a manual tuning of the code, because you make the work for the compiler more difficult. This is not true for inefficient algorithms, they have to be avoided in any case.   

Fixed Size of Data Structures

The next problem is today not an quite often seen anti-pattern. Modern collections frameworks help to avoid fixed size of data structures, but in general assumptions about a frequency of an operation and/or number of data sets are dangerous. Especially the scalability of algorithms may be poorer than expected, if you test just with a limited size of data.

// Pascal - Sample Code #2: Never make hard coded limitations for data structures.
CONST   e_zahl                    = 40 ; 

TYPE
  e_array_type = ARRAY[1..e_zahl] OF ARRAY[1..4] OF integer;    

Mixture of Business Logic and User Interface

Another problem for maintainability is the mixture of business logic and user interface. In the following code piece you see that the visualization class and event handling was in one class with the business logic. This is especially dangerous for validations and the allowed order of execution. 

// Pascal - Sample Code #3: Avoid mixing business logic and user interface code, especially for verification and program flow.

  TYPE
  PmyEditWindow = ^TmyEditWindow; 
  TmyEditWindow = object(Teditwindow)
                    Prozente        : PProzentview; 
                    Wertedialogdata : WB_DialogTyp; 
                    
                    PROCEDURE WertebereichsDialog;                     
                    
                    constructor Init(VAR Bounds: TRect; 
                    FileName: FNameStr; ANumber: Integer; 
                    AInputfileGelesen          : boolean ); 
                    
                    constructor Load(VAR S: TStream); 
                    PROCEDURE HandleEvent(VAR Event: TEvent) ; virtual; 
                    FUNCTION  GetPalette  : PPalette         ; virtual; 
                    
                    .
                    .
                    .                  
                    
                  END; 

Avoidable File System Access and Batch Operations

The following coded solves the linear equations for the FEM analysis. For this output files are created, externally solved and the result will be imported again. Replacing this code with a solution without file system access increased the performance by a factor of 100 without any additional effort. 

// Pascal - Sample Code #4: File access is almost always a performance bottleneck.

    BandMatrix_Speichern('K2.mtx',KB); 
    Prozente^.NeueProzente:= 50; 
    Prozente^.drawview; 
    
    Vektor_speichern('F2.mtx',F_aus ); 
    Prozente^.NeueProzente:= 60; 
    Prozente^.drawview; 
    
    DoneSysError; 
    DoneEvents; 
    DoneVideo; 
    DoneMemory; 
    SetMemTop(Ptr(BufHeapPtr, 0)); 
    SwapVectors; 
    PrintStr('Tippe EXIT um GRAPHIK fortzusetzen.'); 
    Exec(GetEnv('COMSPEC'), '/C solve.bat'); 
    SwapVectors; 
    SetMemTop(Ptr(BufHeapEnd, 0)); 
    InitMemory; 
    InitVideo; 
    InitEvents; 
    InitSysError; 
    application^.redraw; 
    
    Prozente^.NeueProzente:= 100; 
    Prozente^.drawview; 
    Vektor_lesen('F.mtx',F_aus ,'D.mtx',d_aus ); 
    Prozente^.done; 

File system access for data exchange and/or logging is very time consuming. 

Recommendations

  • You can learn a lot from your own old and buggy code. Taking code you wrote as a beginner in software development is for sure an extreme sample, but I see these anti-pattern in my daily work. 
  • Showing young developers the mistakes we made (when we have been Juniors) is sometimes more helpful than just referring to a text book and/or code guideline.

Please, do not hesitate to contact me if you have any ideas for improvement.

Change History

 Revision  Date  Author  Description
 1.0  Nov 29, 2012  Markus Sprunck   first version

Google+ Comments

You may press the +1 button to share and/or comment