Introduction
I had to deal a large "tech dept" of my own making: during many years making tiny technical corrections, additions, features, exemptions on a program of my own making. Even during technological upgrades (moving from OS to OS, from computer language to computer language) I never took the time to review the growing code base. It took me a few months but I think I dealt with quite a part of the tech dept, and got to a point where the system got maintainable, even by someone else ...
Background
I am not much of a C# programmer. In my long career I was mainly on systems, databases, everything today called DevOps, and managing a team.
But a program I wrote eons ago (in Pascal), and upgraded through the years to VB6, VB.NET and C# with WindowsForms is alive and kicking. Every year I would spend some time for “new features” (read: define some more variables, stick them in the If..then conditions, test and let go for a year).
The program runs everything concerning evaluation for students, from an answer to a question in a multiple-choice exam to the final marks for M.D. Now it is a strategic asset for 8 schools in my Faculty of Health Sciences. As a “post-pension” part-time programmer, my job is to transfer maintenance to some fresh forces (a C# programmer with extensive experience in OOP).
Refactoring
The program is about 10.000 lines. Absolutely "procedural" in style and concept, with a few hundred global like variables, arrays etc., and many routines, some short, some long.
For example void GetXGradesFromDB
imports the existing grades for students in a course (divided over components, markups, bonus grades, factors etc.) is a 280 lines jungle of convoluted IFS, such as:
if ((qu.ComponentEvalType == 20 && (SXDate ? ?"") != (student.StudentWDate ? ?"")) ||
(qu.ComponentEvalType == 24 && (SXDate ? ?"") != (student.StudentWDate ? ?"")) ||
(!AllStudentsBox.Checked && !AlephBox.Checked &
Convert.ToInt32(0.ToString() + qu.ExamID) != ExamID && ComponentID == XComponentID) ||
(!Marks700Box.Checked && (qu.Grade == 700)) && !(SourceCode == 4) ||
(!Marks700Box.Checked && (qu.Grade == 800 && !(SourceCode == 4))))
{
continue;
}
if ((XExamMoed > ExamMoed) && (SourceCode == 11))
{
continue;
}
Of course, I could break the statements into pieces, create separate functions to check, introduce temp vars, declare constants for “20”, “24”, “700” etc., run as switch etc.
Or (someone else) could rewrite this object oriented, from scratch.
Reading on CodeProject about Dapper https://www.codeproject.com/Tags/dapper and with my much better experience as SQL programmer, I decided to let the heavy work be done in SQL in simple views and import the relevant data only “as is” in C# to a ready-made object.
I clustered the many variables in lists of objects: students, exams, grades, questions, answers, constants, global management vars etc. Each object would hold all relevant variables. For example, Grades
:
using System;
namespace exam123;
public partial class Exam123F {
public class Grades {
internal int ExamID;
internal int ExamNo;
internal int TZ;
internal int ComponentNumber;
internal bool IsFinal;
internal int IncInFinalGrade;
internal int ExamMarkUp;
internal int ExamComponentID;
internal DateTime ExamDateD;
internal int SourceCode;
public int Grade = -1;
internal int RawGrade = -1;
internal int GradeF = -1;
internal int PartX = 0;
internal int ERankUp;
internal int ERankDown;
[…]
}
So the data concerning students’ grades is just a list of objects, with the TZ
(ID Number) and ExamID
as constructors.
On the SQL side I made VIEWs bringing together data on the students, exam components, exams, questions, answers and grades. I went back and forth between SQL and C# to get the names and definitions (bool or int for example) in sync. This needs some SQL experience, for example getting marked the “oldest row” (ERankUP=1
) and the “latest row” (ERankDown=1
).
When wading through the GetXGradesFromDB
I found I am dealing with just four different runs:
- Getting from each student the grade in the first markup in the course for each component
- Getting for each student the last grade in each component in the course
- Running only an analysis on a specific exam
- Retrieving for each student data on the specified exam, and last data for all other components of the course (the default option)
Afterwards the whole 280 lines operation of reading in students’ data was replace
Using Dapper;
string SetSQLForGetXGrades()
{
string sSQL;
if (AlephBox.Checked)
{
sSQL = "select * from vw_ExamGrades_X where CourseID =" + CourseID +
" and IncInFinalGrade=1 " +
" and ERankUp=1 and PartX=1 " +
" order by ID,ComponentID, ExamMarkUp,ExamDate ";
return sSQL;
}
if (OnlyThisExam.Checked)
{
sSQL = "select * from vw_ExamGrades_X where ExamID =" + ExamID;
return sSQL;
}
if (AllStudentsBox.Checked)
{
sSQL = "select * from vw_ExamGrades_XLast where CourseID =" + CourseID
" and IncInFinalGrade=1 and ERankDown =1" +
" order by TZ,ComponentID, ExamMarkUp,ExamDate ";
return sSQL;
}
sSQL = "select * from vw_ExamGrades_X where CourseID =" + CourseID +
" and IncInFinalGrade=1 and (( ComponentID <> " + ThisExam.ComponentID +
" and ERankDown =1)" +
" or ExamID=" + ExamID + ")" +
" order by TZ,ComponentID, ExamMarkUp,ExamDate ";
return sSQL;
}
public void GetXGradesFromDB()
{
string sql = SetSQLForGetXGrades();
List < Grades > GradesTemp = SqlConn.Query < Grades > (sql).ToList();
GradesL.Clear();
GradesL = GradesTemp;
}
Note that on the SQL side I first made “help tables” with characteristics of each component type, each exam type. For example, in some exam types you have to participate, in some you have to pass, we have “bonus marks”, “defense marks”, “exemption marks” etc. So PassX
, MustBe
, NoWeight
etc. are received by joining the help table to the grades view and basically logical ifs “0” or “1”. Irrelevant grades entries are already filtered out at the SQL level.
Without a single “for loop” I get all the data I need in the object.
Achieved:
- Work only with the relevant data in the program.
- Reading in data in a few lines, without messing around, without nulls etc, complicated conversions from db to variables.
- Removing tons of IF’s
- Simplifying many other ifs
- Most fors replaced by foreach running through a list of objects
- Instead of endlessly looping through arrays to find what I need, zoom to the needed object
using .Where(s=> S.TZ== ..)
lambda statements - Easy traceable objects, with watch list and breakpoints in Visual Studio
Todo List
- Run a ton of tests and set the new version to production.
- Decide on custom reports. These are generated in HTML, applying for loops for the dynamically decided amount of columns needed. Probably should move it to report-generator and not mess around with simplifying the code.
- And now sit with the team and see if they can take over.
But the package stays “procedural”, not OO. It would probably take two years or so to rewrite. The software is very stable, so not worthwhile the effort I guess.
Points of Interest
I am very impressed with CodeProject articles. For example the "Dapper" and Lambda articles got me on track.
Globally many many major software programs are even more ancient than mine (in banks, airlines etc.) It might be important to refactor these with technologies as mentioned here, so extend the practical life, and vastly improve maintainability. Trying to rebuild from scratch (OO of course) is very expensive and fails more often than we think. Refactoring procedural might be a more practical approach.