code style

[tomholub]

We should have a clear code style. This will allow me to inspect the code easier. The most important goal of the code style is to increase code readability.

1. line width 120 characters (already done)
2. indentation 2 characters or 4 in same cases (in change code indentation to two spaces per indent #351)
3. Variable names:
   • Use abbreviations aggressively if the meaning remains clear. For example, generalMessageDetails could be named msgDetails, as Msg is clear to mean Message. Same with Forward -> Fwd, Directory -> Dir, Reference -> Ref, Constant -> Const, Application -> App, Original -> Orig, Generate -> Gen, Command -> Cmd, Attribute -> Attr, Property -> Prop, Destination -> Dest, Connection -> Conn, Database -> Db, Credentials -> Creds, Account -> Acct, FlowCrypt -> Fc, Session -> Sess, Option -> Opt. But make sure not to shorten less common words that could be confused. For example, Connection -> Conn but leave Connect as full word. Same with Optional, etc.
   • Skip words that do not add a meaningful distinction. generalMsgDetails -> msgDetails, msgDaoSource -> msgDao
4. English language improvements:
   • is at the beginning should generally follow with ing or ed if there is a verb at the end: isThreadAlreadyWork() -> isThreadAlreadyWorking(). But even better, isThreadBusy(). Also isDebugEnable -> isDebugEnabled
   • ifNeed should be spelled as ifNeeded.
   • WithOut should be spelled as Without, DataBase should be spelled as Database, but even better Db
5. Use e for Exception name when catching, eg } catch (MessagingException e) {, but only if there is only one level of exception catching (no nested try/catch within each other). When you have nested catch/try blocks, give exceptions meaningful names (not e and e1!), eg:

// this is ok
try {
  callSomeApi();
} catch (MessagingException e) {
  showRetryButton(e);
}

// this is ok
try {
  try {
    Info info = getInfoFromDb();
  } catch (DbCorruptedException dbError) {
    reportToAcra(dbError);
    renderError(dbError)
    return;
  }
  callSomeApi();
} catch (MessagingException msgError) {
  renderRetryButton(msgError);
}

// this is NOT ok
try {
  try {
    Info info = getInfoFromDb();
  } catch (DbCorruptedException e) {
    reportToAcra(e);
    renderError(e)
    return;
  }
  callSomeApi();
} catch (MessagingException e1) {
  renderRetryButton(e1);
}

6. Avoid For if that can make the name shorter. getSessionForAccountDao -> getAccountDaoSession. (It could be shortened further as getAcctDaoSess)

7. Maximum 8 nested block indents, except when the lines are very short and simple. Create a new method and call it if nested blocks get too deep. Eg this is not good (11 indents).

    public LoaderResult loadInBackground() {
        List<KeyDetails> acceptedKeysList = new ArrayList<>();
        try {
            KeyStoreCryptoManager keyStoreCryptoManager = new KeyStoreCryptoManager(getContext());
            Js js = new Js(getContext(), null);
            for (KeyDetails keyDetails : privateKeyDetailsList) {
                String armoredPrivateKey = keyDetails.getValue();
                String normalizedArmoredKey = js.crypto_key_normalize(armoredPrivateKey);

                PgpKey pgpKey = js.crypto_key_read(normalizedArmoredKey);
                V8Object v8Object = js.crypto_key_decrypt(pgpKey, passphrase);

                if (pgpKey.isPrivate()) {
                    if (v8Object != null && v8Object.getBoolean(KEY_SUCCESS)) {
                        if (!keysDaoSource.isKeyExist(getContext(), pgpKey.getLongid())) {
                            Uri uri = keysDaoSource.addRow(getContext(),
                                    KeysDao.generateKeysDao(keyStoreCryptoManager, keyDetails, pgpKey, passphrase));

                            PgpContact[] pgpContacts = pgpKey.getUserIds();
                            List<Pair<String, String>> pairs = new ArrayList<>();
                            if (pgpContacts != null) {
                                ContactsDaoSource contactsDaoSource = new ContactsDaoSource();

                                for (PgpContact pgpContact : pgpContacts) {
                                    if (pgpContact != null) {
                                        PgpKey publicKey = pgpKey.toPublic();
                                        pgpContact.setPubkey(publicKey.armor());
                                        if (js.str_is_email_valid(pgpContact.getEmail()) &&
                                                contactsDaoSource.getPgpContact(getContext(), pgpContact.getEmail())
                                                        == null) {
                                            new ContactsDaoSource().addRow(getContext(), pgpContact);

Instead you should remove chunks and make them into methods, eg:

public void processAndSavePgpContacts(pgpContacts: PgpContact[]) {
  ContactsDaoSource contactsDao = new ContactsDaoSource();
  for (PgpContact pgpContact : pgpContacts) {
    if (pgpContact != null) {
      PgpKey publicKey = pgpKey.toPublic();
      pgpContact.setPubkey(publicKey.armor());
      String email = pgpContact.getEmail();
      if (js.str_is_email_valid(email) && contactsDao.getPgpContact(getContext(), email) == null) {
        new ContactsDaoSource().addRow(getContext(), pgpContact);
        // todo-DenBond7 Need to resolve a situation with different public keys.
        // For example we can have a situation when we have to different public
        // keys with the same email
      }
      pairs.add(Pair.create(pgpKey.getLongid(), email));
    }
  }
}

// ...

if (pgpContacts != null) {
  processAndSavePgpContacts(pgpContacts);
}

8. Save a value into a shorter variable if it repeats in a line and it would cause the line to not wrap, especially if statements. This is bad:

      if (js.str_is_email_valid(pgpContact.getEmail()) && contactsDao.getPgpContact(getContext(), pgpContact.getEmail())
              == null) {
        new ContactsDaoSource().addRow(getContext(), pgpContact);
      }

This is good:

      String email = pgpContact.getEmail();
      if (js.str_is_email_valid(email) && contactsDao.getPgpContact(getContext(), email) == null) {
        new ContactsDaoSource().addRow(getContext(), pgpContact);
      }

The following are recommended - up to discussion - let me know what you think

9. object properties related to count would start with lowercase n: countOfLoadedMessages -> nLoadedMessages, countOfNewMessages -> nNewMessages while properties representing a number of something (order / position in a list) would be spelled out: messageNumber is correct
   • can start with is, has, does in a way that it makes sense in English: .isFolderHasNoSelectAttribute() -> .doesFolderHaveNoSelectAttr()
   • except where negative name makes a lot of sense (should be rare), always use positive names: o.doesFolderHaveNoSelectAttr() -> !o.doesFolderHaveSelectAttr() and o.isKeyNotExistsInList() -> !o.doesListContainKey()
10. Queue must always be named Q or q, and nothing else can be named Q or q. activeSyncTaskBlockingQueue -> activeSyncTaskBlockingQ
11. Attachment is a frequent word that can even show several times per line with no obvious short form. We could agree to always call it Attachment -> Att and Attachments -> Atts. It's not the most obvious for 3rd party developers, but if we agree on it and are consistent, it will shorten the code considerably.

Most of these rules result in shorter code. Shorter variable, class, property, method and object names mean that more code fits on a line (limit 120), which means that the line is less likely to be wrapped at the end. I find lines that wrap around a lot hard to read. Here is an example of how this can help:

// this spans half of a line
PropertiesHelper.generatePropertiesForDownloadGmailAttachments();
// updated by using code style above
PropsHelper.genPropsForDownloadGmailAttachments();
// but even better, which some shorter wording
PropsHelper.genGmailAttachmentDownloadProps();

// another example
messageDaoSource.updateMessageState(context,
        generalMessageDetails.getEmail(), generalMessageDetails.getLabel(),
        generalMessageDetails.getUid(), MessageState.SENDING);
// becomes
msgDao.updateMsgState(context, msgDetails.getEmail(), msgDetails.getLabel(),
    msgDetails.getUid(), MsgState.SENDING);
// which is much more readable in my opinion

[tomholub]

I just noticed the following, is the new necessary there?

  ContactsDaoSource contactsDao = new ContactsDaoSource();
  for (PgpContact pgpContact : pgpContacts) {
    if (pgpContact != null) {
      PgpKey publicKey = pgpKey.toPublic();
      pgpContact.setPubkey(publicKey.armor());
      String email = pgpContact.getEmail();
      if (js.str_is_email_valid(email) && contactsDao.getPgpContact(getContext(), email) == null) {
        new ContactsDaoSource().addRow(getContext(), pgpContact);
        // todo-DenBond7 Need to resolve a situation with different public keys.
        // For example we can have a situation when we have to different public
        // keys with the same email
      }
      pairs.add(Pair.create(pgpKey.getLongid(), email));
    }
  }

Should maybe be:

  ContactsDaoSource contactsDao = new ContactsDaoSource();
  for (PgpContact pgpContact : pgpContacts) {
    if (pgpContact != null) {
      PgpKey publicKey = pgpKey.toPublic();
      pgpContact.setPubkey(publicKey.armor());
      String email = pgpContact.getEmail();
      if (js.str_is_email_valid(email) && contactsDao.getPgpContact(getContext(), email) == null) {
        // removed "new ContactsDaoSource()"
        contactsDao.addRow(getContext(), pgpContact);
        // todo-DenBond7 Need to resolve a situation with different public keys.
        // For example we can have a situation when we have to different public
        // keys with the same email
      }
      pairs.add(Pair.create(pgpKey.getLongid(), email));
    }
  }

[DenBond7]

I just noticed the following, is the new necessary there?

No, there is not. It's redundant. It's my omission after some code changes.

[tomholub]

Kk. That was just a detail. After we ship 0.6.8, please review the code style and let me know if you have any comments or changes to the code style above.

[tomholub]

12. When nesting IF and FOR blocks, do not duplicate the for loop if the loop is the same and only content changes. Change the structure from IF -> FOR -> command to FOR -> IF -> command.

This is not good:

if (outgoingMessageInfo.getMessageEncryptionType() == MessageEncryptionType.ENCRYPTED) {
  for (AttachmentInfo attachmentInfo : outgoingMessageInfo.getForwardedAttachmentInfoList()) {
    AttachmentInfo attachmentInfoEncrypted = new AttachmentInfo(JavaEmailConstants.FOLDER_OUTBOX, attachmentInfo);
    attachmentInfoEncrypted.setName(attachmentInfoEncrypted.getName() + Constants.PGP_FILE_EXT);
    cachedAttachments.add(attachmentInfoEncrypted);
  }
} else {
  for (AttachmentInfo attachmentInfo : outgoingMessageInfo.getForwardedAttachmentInfoList()) {
    cachedAttachments.add(new AttachmentInfo(JavaEmailConstants.FOLDER_OUTBOX, attachmentInfo));
  }
}

This is better (if the IF statement is very expensive you can save it as boolean variable before the loop):

for (AttachmentInfo attachmentInfo : outgoingMessageInfo.getForwardedAttachmentInfoList()) {
  if (outgoingMessageInfo.getMessageEncryptionType() == MessageEncryptionType.ENCRYPTED) {
    AttachmentInfo attachmentInfoEncrypted = new AttachmentInfo(JavaEmailConstants.FOLDER_OUTBOX, attachmentInfo);
    attachmentInfoEncrypted.setName(attachmentInfoEncrypted.getName() + Constants.PGP_FILE_EXT);
    cachedAttachments.add(attachmentInfoEncrypted);	
  } else {
    cachedAttachments.add(new AttachmentInfo(JavaEmailConstants.FOLDER_OUTBOX, attachmentInfo));
  }
}

The results is 9 lines instead of 11, and the intent is more readable - no need to define the same for loop twice.

[tomholub]

Looking at it, most of these code style rules can be deployed across all projects, they are not very Java specific.

[DenBond7]

@tomholub
I agree with all items except next moments. I will be happy if we won't use single-letter prefixes for the Java names. As for me, it's not easy for reading. I agree that we need to make names more shortly but those names must be readable at a glance.

object properties related to count would start with lowercase n: countOfLoadedMessages -> nLoadedMessages, countOfNewMessages -> nNewMessages while properties representing a number

Could you propose an example without the n prefix?

Queue must always be named Q or q, and nothing else can be named Q or q. activeSyncTaskBlockingQueue -> activeSyncTaskBlockingQ

In my opinion will be better to leave a more readable version like this activeSyncTaskBlockingQueue -> activeQueue

Basically, it's a great idea to create a project code style. And I think we can use checkstyle. There is a plugin for Gradle. I can setup it when we'll approve the code style. It'll help us don't forget use the code style :)

[tomholub]

Thanks for feedback. We can proceed without the n and q suggestions.

For count maybe like this then: countOfNewMessages -> newMsgCount - this looks ok to me

[DenBond7]

newMsgCount

Great! I'll use such logic.

[DenBond7]

@tomholub
We've approved the code style. But basically, the current issue is unnecessary for the current release (as logic improvement or bug fixing etc.). I think we can move it to the next release. In this case, we don't need to wait while I'll complete it. Because the code refactoring will take a couple days. It's a last issue in the current milestone.

What do you think?