The most common problems I see are inappropriate naming and long functions and insufficient encapsulation.
I'll go over these one by one. Note that most of my opinions on this matter come from Clean Code by Uncle Bob: Book Link | YouTube Lecture Link (which I can only recommend)
I like the philosophy that code should be self documenting. Appropriate naming helps a lot with that, and inappropriate names can confuse the reader - which may be you in 2 weeks. We've all been there.
So how do I name appropriately?
- I usually always write out abbreviations unless they are well known in my context. Sorry C devs.
- I never name my variables
tmp if they live longer than two lines of code.
- It doesn't matter which naming convention you use: camelCase, PascalCase, lowercase, lowercase_separated_by_underscores, ... or whatever, as long as you do it consistently. And your team agrees on it, if you're working in a team.
And probably most importantly:
- I name my variables after their purpose.
- Variables are objects, their names are nouns.
- I name my functions after their intent, not their implementation details.
- Functions do stuff, their names are verbs.
A small exception to the noun / verb thing are booleans: I usually write them as
user_is_logged_in. I try to avoid negative terms in these conditions e.g.
user_is_logged_out - although
user_is_logged_out sometimes is justifiable.
Often, when I see code to be reviewed, it has at least one long function. Half the time, this long functions consists of blocks of code with a comment above it, that describes what the block does. Usually, that block can be extracted into its own function. If you use Jetbrains products like IntelliJ, the IDE can do most of the work for you: mark the block to be extracted -> right click ->
Extract. If not, refactoring.guru/extract-method has a neat example and explanation.
If you don't have these conveniently placed comments, you'll have to do decide which blocks can be extracted.
I like to use these general rules of thumb:
- Is the method longer than 5 lines? -> Extract some of it.
- Does the method have more than one level of indention? E.g. nested ifs or loops? -> Extract until it only has one level indention.
Why would I do that? Now I have to look at more than one place for what my code does! I skip around in files and lines all the time now, debugging became a nightmare!
Well first of, good that you're using a debugger. If not, learn how to.
Second, debugging becomes easier for the same reasons that your code is now easier to read and understand (if you've named appropriately): If you only need to read the name of a function to understand what it does, you can skip reading the implementation, because you know what it does. You only need to read it if you also need to know how it does that. In the same way, parts of your code became now skippable during debugging, where you don't need to confirm every step. I'd argue debugging became easier.
Granted, this approach usually works best when paired with a lot of tests - which come free when doing TDD - or just being very confident in your code. I'd argue the tests are usually better, but for smaller projects - which I know will stay small - confidence is enough for me.
For more info, refactoring.guru/smells/long-method is a good resource.
This usually comes back to the question Is my method / class doing one thing only?
I really like Sandy Metz' squint test when I first look at code, because it tells me multiple things:
- nested indentation (see the previous paragraph)
- colors in the code
Now why would the colors in the code matter? Assuming you use an IDE which colors variables, members, functions and constants differently.
It matters because the more colorful your code is, the more you mix responsibilities and therefore purposes of your code. The best example for this are magic numbers or strings: If your code is doing high level calculations and also checking for a specific string in the same line, you've mixed responsibilities.
Another pattern I regularly see is mixing IO with logic: A master class that takes input, calculates some logic and outputs some intermediary results. The final result usually is printed out somewhere separately - which is good. However, the master class is clearly doing more than one thing. So how do we make our classes and functions do one thing only?
Architecture patterns exist for this very reason - to separate concerns, or responsibilities. Well known examples include Model-View-Controller (MVC) and similar patterns. I like MVVM.
The gist of all these patterns is to separate logic from IO (and persistence if needed). They do this by creating a layer for the logic and a layer for the IO and some intermediary layer that connects the other two layers. It's important to know that the logic layer should know nothing of the IO layer. The logic doesn't need to know how its data gets viewed, be it via command line or JPanel. The IO layer doesn't need to know the data types the logic uses to calculate, it needs to know the numbers and strings it should display. These are sometimes conveniently grouped into structures that the logic uses, and I think it's okay if the IO also uses these structures to display matching data.
However, all interaction between these layers should go through the intermediate layer, which provides intent functions for the IO and updates the IO if changes in the model occurs.
- IO is a GUI and there's a button to draw a random card to your hand.
- The intermediate layer provides a function
drawToHand(), which is called by the GUI when the button is pressed.
drawToHand() connects to the model and updates it (via another function call, this time in the model. The intermediate layer does not do application logic, it only transforms data and notifies the other layers.).
- when the model update is complete, the intermediate layer informs or updates the IO layer, which then displays the new data.