Note: This is in the "slush pile" of unfinished articles, not only is it not finished, but it's probably going to change a lot or be deleted.
These are usually called "code smells" because they're almost always bad no matter where you find them, but remember that Anything Is Appropriate As Long As You Know What You're Doing (TM).
These don't mean you're a Bad Programmer, though, because they tend to arise in the normal development cycle like lint balls on a sweater. They accumulate into "Technical Debt" that slow down future development.
The only thing that makes you a bad programmer is not spotting these and refactoring them before you shift to a maintenance cycle.
No architectural model / Inconsistent use of models
- Class names and hierarchy suggest you're using MVC but you've dropped UI code in a model object, business logic in the UI and data in the controller
Maintenance and extension goes faster and more reliably if you pick a major architectural model and stick to it. That might be MVC, MVVC, MOVE
, REPL or whatever.
Carrying too many bags around / Abusing the Bell-Hop
- Your method signature includes parameters that aren't directly used by that method, but have to be ferried to something your method relies on, e.g.: you're passing your
generateReport() method a
BaseURL parameter that's needed by a template processor, but only if the user has selected HTML for the output
- Your core logic routines are querying the state of UI controls directly (especially if the UI runs on another thread and can change while you're still working)
- You resort to global variables or singletons to fetch values that originate early in a nested chain of functions, but aren't used until the end of the chain
- You have a dozen independent lookup tables (in RAM or files) and keep re-querying the same value in each method
Consider creating a class for each major function or "verb" of your program and using those as the primitives that encapsulate the program's state. A search engine might have
SearchResult; an ATM might have
WithdrawlResult, while the bank would wrap those in a
Transaction along with
TransferRequest. These classes would have many properties that capture both direct input and environmental details such as the account number, authentication data, time and location of an ATM withdrawl.
Apple does this in iOS with GameKit, too: there's a
GKMatchRequest that encapsulates the parameters for a player search, and once a suitable player is found they're sent a
GKInvite object that identifies the invitee and how the game is hosted.
In Spent Better
I created a class to represent a "Request" that encapsulated the amount, spending frequency, time and all other environmental values that could be pertinent. The major functions consumed instances of
and returned instances of
. Deeper in the machinery I can break-up a large concept into smaller ones and implement the conversions in the constructors or static methods of the specialized types. For example,
is a static method implemented in the
class that returns an object I can then pass to an amortization function. I can implement
depending on whether I need to generate the table ahead of time or after the initial results have been computed.
You obviously needn't take this to the extreme; there's no need for a math library with RaiseToPowerRequest and RaiseToPowerResult classes. I use the following criteria:
- The function depends on environmental values (time, location, temperature, etc.) or multiple values from the UI. Say you've a method that calculates driving directions, and you implemented it as
GPS.DrivingDirections(Location start, Location end), but now the user wants to switch off toll roads and ferries, but only for that one trip
- The user would consider it a complete event for the level that the software runs at: "KeyDown" is a complete event for an operating system, but not a word processor that consumes many of them to make a single document. "Print" and "Email" are complete events for a word processor, but not for an order manager that prints and emails multiple receipts in the context of a Sale or Return, and a Sale or Return might be partial events to something else
- You've just added a second or third parameter that seems to belong to the first, e.g.: area code, phone number and extension; IP address, interface, and subnet
- Code is easier to read because your method prototypes and function calls have fewer arguments
- Code is easier to reason about because major concepts are now expressed in meaningful names
- Some classes of bugs can be eliminated if the compiler does type checking
- Easier to separate the UI's code from the guts
- Program doesn't break if the user changes any of the UI controls while calculations are still happening in the background
- Values that are only used at the end of a function chain are available without polluting the prototypes of earlier methods
- It's easier to add new parameters and capture more of the environment, such as the GPS coordinates you now want to attach to a blog post
- Easier to serialize and send to the wire/disk or memoize for a history/undo feature
- Uses less memory on the stack for values that aren't going to change before the job is complete
- A function that performs a calculation but embeds the result in a formatted string, e.g.: a
FormatToLocalCurrency() method that also looks up and multiplies the value by the current exchange rate
- Using arrays or XML as ad-hoc data structures within the program
Too many side effects
- Your ConvertToLocalCurrency() method also updates a database with the latest exchange rate
- A method that processes data also directly updates the UI
1: Break up methods that change or retrieve more than one persistent value into multiple methods that each do only one thing; one method that fetches a value from somewhere, another that modifies it, another that stores it back somewhere, and one that updates the UI.
2: Use Events or Delegates to give the responsibility of updating the UI/database/filesystem/etc to other code.