akvo / akvo-flow

A data collection and monitoring tool that works anywhere.
http://akvo.org/products/akvoflow/
GNU Affero General Public License v3.0
65 stars 31 forks source link

Code quality and readability review #3928

Closed lutzseverino closed 1 year ago

lutzseverino commented 2 years ago

Code quality and readability review

This PR contains certain quality improvements that don't tackle on changing the approach, but rather readability.

Problem

Code clarity and readability could be improved upon on some parts of the code I'm going to work on.

Solution

Automatically format with the IDE, cleanup the logic, and many other micro-improvements that make the code more readable and of better quality overall.

Examples

Use of ternary operators

This logic may be unnecessary or overcomplicated, but I'm not worrying about that right now.

public String getValue() {
    if (value != null) {
        return value;
    }

    if (valueText != null) {
        return valueText.getValue();
    }

    return null;
}

Replacing that with a ternary one-liner improves readability and makes it easier for a future contributor to apply a different approach or verify this one.

public String getValue() {
    return value == null ? (valueText == null ? null : valueText.getValue()) : value;
}

Switches

If-else chains to switches…

if ("buildMap".equals(action)) {
    // empty
    } else if ("purgeExpiredSurveys".equals(action)) {
        purgeExpiredSurveys();
    } else if ("purgeOrphanJobQueueRecords".equals(action)) {
        purgeOrphanJobQueueRecords();
// ...
switch (action) {
    case "buildMap":
        // empty
        break;
    case "purgeExpiredSurveys":
        purgeExpiredSurveys();
        break;
    case "purgeOrphanJobQueueRecords":
        purgeOrphanJobQueueRecords();
// ...

Line-breaks

And convenient line-breaks for a massive readability boost.

private void extractImageFileGeotags() {
    Calendar deadline = Calendar.getInstance();
    deadline.add(Calendar.MONTH, ONE_MONTH_AGO);
    log.info("Starting scan for image answers, newer than: " + deadline.getTime());
    QuestionAnswerStoreDao qaDao = new QuestionAnswerStoreDao();
    String cursor = "";
    int json = 0;
    int nonjson = 0;
    Media media;
    // ...
private void extractImageFileGeotags() {
    Date date = this.getModifiedDate(Calendar.MONTH, -1);

    log.info("Starting scan for image answers, newer than: " + date);

    int json = 0;
    int nonJson = 0;

    QuestionAnswerStoreDao qaDao = new QuestionAnswerStoreDao();
    String cursor = "";
    Media media;
    // ...

Extract repeated code

Additionally, some code repetition was present and removed. Not everywhere though, some instances were not worth to change due to possible changes in the future. This logic was present several times, including the #getTime call. This can all be effectively abstracted.

private void purgeOldMessages() {
    Calendar deadline = Calendar.getInstance();
    deadline.add(Calendar.YEAR, ONE_YEAR_AGO);
    log.info("Starting scan for Message entries older than: " + deadline.getTime());
// ...
private void purgeOldMessages() {
    Date date = this.getModifiedDate(Calendar.YEAR, -1);

    log.info("Starting scan for Message entries older than: " + date);

A new approach may still be desired, but once again, that's not what I'm focusing on right

lutzseverino commented 2 years ago

Created a draft to ask the following.

The BaseDAO contains a piece of code that is repeated 3 times.

if (cursorString != null && !cursorString.trim().equalsIgnoreCase(Constants.ALL_RESULTS)) {
    Cursor cursor = Cursor.fromWebSafeString(cursorString);
    Map<String, Object> extensionMap = new HashMap<>();
    extensionMap.put(JDOCursorHelper.CURSOR_EXTENSION, cursor);
    query.setExtensions(extensionMap);
}

If it were to be extracted, what would be an appropriate method name?