Clarify your code with comments if it is not absolutely straightforward.
Sprinkle your code with relevant comments explaining what the code is supposed to accomplish. Comments should help the reader over the difficult spots. Thy will not only help other developers to understand your code, but will also be very useful for yourself: investigating a production issue after many years, will you remember what your tricky code does?
Use comments as headers of different code fragments to visually divide them.
While your inline comments should assist the next developer in fixing or extending your code, your header comments should give any programmer enough information to use the code fragment without having to read it. As an example, I will provide real code, written by me in one of the projects. You can spend a few seconds and understand what is going on by only reading the header comments!
CREATE PROCEDURE o_lht_manage_qefs_21a_21b @pol_no numeric(12) ,@pol_ver_dt smalldatetime ,@pol_item_ins_no smallint ,@fleet_rating_typ char(1) AS /****************************************************************************************************************************************** When user changes "Fleet Rating" field in "Blanket" window (in a new or existing fleet) and clicks OK, this proc:
1. Sets the new "Fleet Rating" to all other blankets in the policy as well (to keep them in sync).
2. Manages existence of QEFs 21a and 21b in the policy depending on the chosen value of "Fleet Rating": 'a' ("21a") -> adds QEF 21a to the policy (and deletes QEF 21b if exists). 'b' ("21b") -> adds QEF 21b to the policy (and deletes QEF 21a if exists). 'n' ("None") -> deletes any of the QEFs 21a or 21b if exists.
REMARKS: * QEFs 21a and 21b are added on the policy level, i.e. synchronized through all the fleets of the policy. * A policy is not allowed to have both 21a and 21b in the same time. * Existence of QEFs 21a and 21b is reflected in two places: in the list of coverages on the Vehicle tab, and as a value of "Fleet Rating" field in "Blanket" window (in DB: ii_veh_lht.fleet_rating_typ). ******************************************************************************************************************************************* Michael Zuskin 12-Feb-2019 Created *******************************************************************************************************************************************/ DECLARE @new_cov_cd smallint ,@other_cov_cd smallint ,@now_dt datetime ,@err_msg varchar(500)
-- Some houskeeping + parameters validation: IF @fleet_rating_typ = 'a' /* "21a" */ BEGIN SET @new_cov_cd = 1436 -- QEF 21a SET @other_cov_cd = 1419 -- QEF 21b END ELSE IF @fleet_rating_typ = 'b' /* "21b" */ BEGIN SET @new_cov_cd = 1419 -- QEF 21b SET @other_cov_cd = 1436 -- QEF 21a END ELSE IF @fleet_rating_typ = 'n' /* "None" */ BEGIN SET @new_cov_cd = NULL END ELSE BEGIN SET @err_msg = 'PROC o_lht_manage_qefs_21a_21b: argument @fleet_rating_typ must be ''a'', ''b'' OR ''n'', not ''' + IsNull(@fleet_rating_typ, 'NULL') + '''' RAISERROR 20001 @err_msg RETURN END
-- Set the new "Fleet Rating" to all other blankets in the policy as well (to keep them in sync): UPDATE ii_veh_lht SET fleet_rating_typ = @fleet_rating_typ WHERE pol_no = @pol_no AND pol_ver_dt = @pol_ver_dt AND pol_item_ins_no <> @pol_item_ins_no -- exclude the current fleet to prevent DB error "Row changed between retrieve and update"
-- If user is deleting an exisiting QEF (21a or 21b) from the policy: IF @fleet_rating_typ = 'n' /* "None" */ BEGIN DELETE FROM pol_sef WHERE pol_no = @pol_no AND pol_ver_dt = @pol_ver_dt AND cov_cd IN (1436 /* QEF 21a */, 1419 /* QEF 21b */)
RETURN -- we are done - nothing should be inserted END
-- If this code reached, user is adding 21a or 21b to the policy.
-- Delete the OTHER QEF, if exists (only one of 21a and 21b is allowed - not both): DELETE FROM pol_sef WHERE pol_no = @pol_no AND pol_ver_dt = @pol_ver_dt AND cov_cd = @other_cov_cd
-- Maybe, the new QEF already exists in the policy: IF EXISTS (SELECT 'x' FROM pol_sef WHERE pol_no = @pol_no AND pol_ver_dt = @pol_ver_dt AND cov_cd = @new_cov_cd) RETURN -- we are done - it's already there
-- If this code reached, the new QEF doesn't exist in the policy, so let's insert it: SET @now_dt = GetDate() EXEC i_sa_pol_sef @pol_no = @pol_no, @pol_ver_dt = @pol_ver_dt, @pol_item_ins_no = 0, -- policy level @cov_cd = @new_cov_cd, @pde_cd = NULL, @delivery_dt = NULL, @occopr_cd = NULL, @pol_cov_option_no = NULL, @sef_add_dt = @now_dt
RETURN
Write header comments before writing the code.
Comments can help you not only understand existing methods, but also create new methods. Prior to writing any executable line of code, write a comment before each future code fragment which will perform a separate logical task (as if you would comment an existing code). When the method will have been written, those comments will serve as the fragment's titles, but now they help to write code. They will force you to think what to do before writing the executable lines, so, later, you don't need to re-write your code if you discover, that the logic is incorrect. Below is an example of an initial skeleton, built of comments, for a function which calculates a new balance for a customer:
// If it exceeds allowed overdraft then return original balance:
// If this code reached then new amount is ok - return it:
}
An that moment it's easier to concentrate on the "upper-level logic" - you simply describe what the function is doing using the regular human language. After the "comments skeleton" has been created, start writing executed code (each fragment just after its corresponding comment).
Don't leave comments if they are not absolutely necessary.
Write as little comments as possible. Don't just echo the code with comments! As the first sentence of this topic says, Clarify your code with comments if it is not absolutely straightforward - not always you have time. I saw comments like 'Declare variables:' just before a variables declaration section and 'Call function XXX:' just before calling function XXX ! They restate the obvious and provide no new information. These comments are nothing but noise. And too many such comments are so noisy that we learn to ignore them. As we read through code, our eyes simply skip over them. So, we must ensure, that the comments are always valuable. Make every comment count!
Here is an example of comments which supply no new information:
-- Delete the OTHER QEF, if exists (only one of 21a and 21b is allowed - not both): DELETE FROM pol_sef WHERE pol_no = @pol_no AND pol_ver_dt = @pol_ver_dt AND cov_cd = @other_cov_cd
Don't use comments as a substitution for well-named variables and methods, and for correct methods organization.
Comments can be a red flag! When a comment becomes too involved, ask yourself whether the code itself is at fault. Even if bad code is explained, it remains bad. Remember:
The best comment is the comment you found a way not to write!
Just a few lifehacks in that regard:
• Give variables and methods exact names which clearly reveal intent. Name a variable daysAferLastPurchase (rather than days); name a method getFilterBySelectedRowInSummaryScreen() (rather than getMainFilter()). They are self-commented, so you have nothing to add.
• Put expression result in a well-named Boolean variable rather than explain in a comment what that expression calculates (see Short conditions in IFs). An example from my real application in Kotlin:
// If the current language has no translation in the application, use English: if (currLang !in dropDownPref.entryValues) currLang = Lang.ENGLISH
• Before writing a header comment to visually mark a code fragment, consider refactoring that fragment into a method with a self-explanatory name (see Keep functions simple).
I remember a couple of cases when I wrote a whole story as a comment, and that forced me to look critically on my code and to re-organize some methods, making the module simpler. The lesson I learned: too many comments can signal, that I do things in a more complicated way, than possible.
Update comments if necessary when the code they clarify changes.
Comments can begin to lie if the code around them changes, so keep them in sync.
Re: /* Comments */
Posted: 03 Jul 2019, 12:20
by Ursego
From the book "Clean Code":
Explain Yourself in Code
There are certainly times when code makes a poor vehicle for explanation. Unfortunately, many programmers have taken this to mean that code is seldom, if ever, a good means for explanation. This is patently false. Which would you rather see? This:
It takes only a few seconds of thought to explain most of your intent in code. In many cases it's simply a matter of creating a function that says the same thing as the comment you want to write.
From the book "97 Things Every Programmer Should Know":
Comment Only What the Code Cannot Say
The difference between theory and practice is greater in practice than it is in theory - an observation that certainly applies to comments. In theory, the general idea of commenting code sounds like a worthy one: offer the reader detail, an explanation of what's going on. What could be more helpful than being helpful? In practice, however, comments often become a blight. As with any other form of writing, there is a skill to writing good comments. Much of the skill is in knowing when not to write them.
When code is ill-formed, compilers, interpreters, and other tools will be sure to object. If the code is in some way functionally incorrect, reviews, static analysis, tests, and day-to-day use in a production environment will flush most bugs out. But what about comments? In The Elements of Programming Style (Computing McGraw-Hill, PDF), Kernighan and Plauger note that "a comment is of zero (or negative) value if it is wrong". And yet such comments often litter and survive in a codebase in a way that coding errors never could. They provide a constant source of distraction and misinformation, a subtle but constant drag on a programmer’s thinking.
What of comments that are not technically wrong, but add no value to the code? Such comments are noise. Comments that parrot the code offer nothing extra to the reader - stating something once in code and again in natural language does not make it any truer or more real. Commented-out code is not executable code, so it has no useful effect for either reader or runtime. It also becomes stale very quickly. Version-related comments and commented-out code try to address questions of versioning and history. These questions have already been answered (far more effectively) by version control tools.
A prevalence of noisy comments and incorrect comments in a codebase encourages programmers to ignore all comments, either by skipping past them or by taking active measures to hide them. Programmers are resourceful and will route around anything perceived to be damage: folding comments up; switching coloring scheme so that comments and the background are the same color; scripting to filter out comments. To save a codebase from such misapplications of programmer ingenuity, and to reduce the risk of overlooking any comments of genuine value, comments should be treated as though they were code. Each comment should add some value for the reader, otherwise it is waste that should be removed or rewritten.
What then qualifies as value? Comments should say something code does not and cannot say. A comment explaining what a piece of code should already say is an invitation to change code structure or coding conventions so the code speaks for itself. Instead of compensating for poor method or class names, rename them. Instead of commenting sections in long functions, extract smaller functions whose names capture the former sections’ intent. Try to express as much as possible through code. Any shortfall between what you can express in code and what you would like to express in total becomes a plausible candidate for a useful comment. Comment what the code cannot say, not simply what it does not say.
Re: /* Comments */
Posted: 01 Nov 2019, 12:49
by Ursego
Just an example how I use comments - taken from a real class I created:
public class YearlyReport implements IReport { private static final boolean DEBUG_MODE = true; // true - process only the first 15 rows of Excel; false - production mode, process all rows
// Zoho modules: https://crm.zoho.com/crm/org639870014/settings/api/modules private static final String ZOHO_MODULE_NAME__ACCOUNTS = "Accounts"; private static final String ZOHO_MODULE_NAME__YEARLY_RESULTS = "Yearly_Results";
private static final int EXCEL_CELL_NUM__ACCOUNT_NUMBER = 0; private static final int EXCEL_CELL_NUM__SOURCE_APPLICATION = 1; private static final int EXCEL_CELL_NUM__MASTER_BROKER_NUMBER = 2; private static final int EXCEL_CELL_NUM__MASTER_BROKER_NAME = 3; private static final int EXCEL_CELL_NUM__BROKER_NUMBER = 4; private static final int EXCEL_CELL_NUM__BROKER_NAME = 5; private static final int EXCEL_CELL_NUM__CLASS = 6; private static final int EXCEL_CELL_NUM__WRITTEN_PREMIUM = 7; private static final int EXCEL_CELL_NUM__EARNED_PREMIUM = 8; private static final int EXCEL_CELL_NUM__INCURRED_AMOUNT = 9;
private static final HashMap<Integer, String> zohoFieldNameFirstPartByExcelCellNum = new HashMap<>(); static { zohoFieldNameFirstPartByExcelCellNum.put(EXCEL_CELL_NUM__WRITTEN_PREMIUM, "Written_Premium_"); zohoFieldNameFirstPartByExcelCellNum.put(EXCEL_CELL_NUM__EARNED_PREMIUM, "Earned_Premium_"); zohoFieldNameFirstPartByExcelCellNum.put(EXCEL_CELL_NUM__INCURRED_AMOUNT, "Incurred_Amount_"); }
// zohoClassByExcelClass maps Excel Classes (as they appear in the Class column of Excel - like "Commercial Property") to Zoho Classes // (as they appear in Zoho API - like "Commercial_Property"; see all the Zoho API fields here: https://bit.ly/2OXPHxB). // In most cases (but not always!!!), the Zoho Class is the same as the Excel Class, but with underscores instead of spaces. // Zoho Classes are used as the 2nd part of some API fields names which are built by the pattern "[field]_[class]", where: // [field] - the 1st part: one of the three: "Earned_Premium", "Written_Premium" or "Loss_Ratio"; // [class] - the 2nd part: "Commercial_Property", "Personal_Property", etc. // For example: Earned_Premium_Commercial_Property. In this case, this HashMap will return "Commercial_Property" ready for concatenation with class. private static final HashMap<String, String> zohoClassByExcelClass = new HashMap<>(); static { zohoClassByExcelClass.put("Commercial Property", "Commercial_Property"); zohoClassByExcelClass.put("Commercial Liability", "Commercial_Liability"); zohoClassByExcelClass.put("Commercial Auto", "Commercial_Automobile"); zohoClassByExcelClass.put("Personal Property", "Personal_Property"); zohoClassByExcelClass.put("Personal Liability", "Personal_Liability"); zohoClassByExcelClass.put("Motorcycle", "Motorcycle"); zohoClassByExcelClass.put("Long Haul Trucking", "Long_Haul_Trucking"); zohoClassByExcelClass.put("Personal Auto", "Personal_Auto"); zohoClassByExcelClass.put("All Terrain Vehicles", "Non_Standard_Automobile"); zohoClassByExcelClass.put("Antique Auto", "Non_Standard_Automobile"); zohoClassByExcelClass.put("Snow Vehicles", "Non_Standard_Automobile"); zohoClassByExcelClass.put("Motor Homes", "Motorhome_Travel_Trailer"); zohoClassByExcelClass.put("Trailer Homes", "Motorhome_Travel_Trailer"); zohoClassByExcelClass.put("Accident Sickness", "Accident_Sickness"); zohoClassByExcelClass.put("General Liability", "General_Liability"); zohoClassByExcelClass.put("Specialty", "Specialty"); zohoClassByExcelClass.put("Warranty", "Warranty"); zohoClassByExcelClass.put("Surety", "Surety"); zohoClassByExcelClass.put("Legal", "Legal"); }
/***********************************************************************************************************************************************************/ @Override public List<ZCRMRecord> convert (XSSFWorkbook workbook) { HashMap<String, ZCRMRecord> zohoRecordByAccountNumberHashMap = new HashMap<>(); XSSFSheet sheet = workbook.getSheetAt(0); // the 1st sheet containing data for report String reportYear = this.getReportYear(workbook);
int excelRowCount = sheet.getPhysicalNumberOfRows(); for (int i = 2; i < excelRowCount; i++) { if (DEBUG_MODE && i == 18) break; XSSFRow excelRow = sheet.getRow(i); this.convertExcelRow(workbook, excelRow, reportYear, /* into the "values" part of >>> */ zohoRecordByAccountNumberHashMap); }
ArrayList<ZCRMRecord> recordsArrayList = new ArrayList<ZCRMRecord>(zohoRecordByAccountNumberHashMap.values()); MainApplication.getLogger().info("Converted " + recordsArrayList.size() + " rows from sheet " + sheet.getSheetName()); return recordsArrayList; }
// For each broker, many rows can exist in Excel (with different Class!!!). But we send each broker as one record. // The more records (Classes) exist for a broker, the more class-related fields are added to that broker's record. // If this loop encounters an already processed broker, it adds new Class-related fields to the previously created record of that broker. // For example: if broker A has 7 rows in Excel, we add 7 fields "Earned_Premium_[class]" to the same record in 7 iterations of this loop. String accountNumber = excelRow.getCell(EXCEL_CELL_NUM__ACCOUNT_NUMBER).getStringCellValue(); ZCRMRecord zohoRecord = zohoRecordByAccountNumberHashMap.get(accountNumber); if (zohoRecord == null /* it's first time the loop encounters this broker */) { zohoRecord = new ZCRMRecord(ZOHO_MODULE_NAME__YEARLY_RESULTS); zohoRecordByAccountNumberHashMap.put(accountNumber, zohoRecord);
// The combination of Account Number, Source Application and Class is unique in the Excel file (logically, it's like a PK). // That means, that a same combination of Account Number & Class can appear in the Excel more than once - under different Source Applications. // The processed dollar amount field could already be processed (added to Zoho record) for this Account Number previously, // under (an)other source Source Application(s) - let's see if an already inserted value exists: double valueExistingInRecord = (double) zohoRecord.getFieldValue(zohoFieldName); if (valueExistingInRecord != 0) { // Don't override the already inserted value. Instead, summarize it with the value we are copying from Excel now: valueToSet = valueExistingInRecord + valueFromExcel; } } catch (IllegalStateException | NumberFormatException e) { MainApplication.getLogger().warn("Cell: (" + (excelRow.getRowNum() + 1) + ", " + (excelCellNum + 1) + "). Error: " + e.getMessage()); } catch (ZCRMException e) { String accountNumber = excelRow.getCell(EXCEL_CELL_NUM__ACCOUNT_NUMBER).getStringCellValue(); MainApplication.getLogger().warn("Faild to get value of " + zohoFieldName + " field in record for account #" + accountNumber + ". Error: " + e.getMessage()); }
-- If an antique vehicle was deleted from the policy, and then the insured decided to re-add it back, -- it cannot be added for a period of 1 year from the day it got deleted: IF NOT EXISTS (SELECT NULL -- if this veh doesn't exist in the current policy version (i.e. this transaction is adding it) FROM ii_veh v, pol_ver_planloc p WHERE v.pol_no = @pol_no AND v.pol_ver_dt = @pol_ver_dt AND v.vin = @vin AND p.pol_no = @pol_no AND p.pol_ver_dt = @pol_ver_dt AND p.pol_ver_planloc_no = v.pol_item_ins_no AND p.chg_sta_cd <> 'D' /* it's 'D' if vehicle existed, then was deleted (becoming 'D'), and now this transaction is re-adding it */) BEGIN -- Obtain the date when the vehicle was deleted from the policy. If no such date exists, the vehicle never existed, so we have nothing to validate. -- The vehicle could be deleted/re-added, deleted/re-added, deleted/re-added many times in the past. But we need to count 1 year after its LAST deletion. SELECT @veh_deletion_dt = NULL SELECT @veh_deletion_dt = Max(pol_ver_dt) -- grab the LAST of all the version dates among the versions... FROM pol_ver WHERE pol_no = @pol_no AND NOT EXISTS (SELECT NULL -- ...in which the vehicle already didn't exist... FROM ii_veh WHERE pol_no = @pol_no AND pol_ver_dt = @pol_ver_dt AND vin = @vin) AND EXISTS (SELECT NULL -- ...but had existed in the version immediately prior to each of them FROM ii_veh v1 WHERE v1.pol_no = @pol_no AND v1.pol_ver_dt = (SELECT Max(v2.pol_ver_dt) -- immediately... FROM ii_veh v2 WHERE v2.pol_no = @pol_no AND v2.pol_ver_dt < v1.pol_ver_dt -- ...prior to each of them AND v2.vin = @vin) AND v1.vin = @vin)
IF @veh_deletion_dt /* <<< it's NULL if veh never existed on policy */ > DateAdd(year, -1, @pol_ver_dt) BEGIN SET @msg = @veh_desc + ' was deleted from policy less than 1 year ago (in ' + convert(varchar, @veh_deletion_dt, 107 /* "mon dd, yy" */) + '); prior to be re-added, it must stay off for a period of 1 year after deleting' EXEC i_ws_pol_ver_msg @pol_no, @pol_ver_dt, @pol_ver_msg_no out, 1, @msg END END