Tuesday, November 25, 2008

Crap code

I am not the most senior, nor experienced coder in the industry, but I am motivated to improve my coding skill by implementing good coding design via strong typing, interfaces, and proper code placement in response to high-level requirements.

That being said, I consider myself a decent coder. I look back on old projects, thinking about what would I have done differently if given the opportunity again. I have received compliments from colleagues and peers (that I both respect and don't respect in regards to their development skill) in regards to my coding style and coding decision making process.

I have taken over an old code base for the new project on which I have begun work. This code base sucks. It easily meets the definition of cruft.

Cruft - Computing jargon for code, data, or software of poor quality.

Poor technical managers often do not realize the importance of good developers. This statement is slightly self-serving, but there's a popular misconception among managers that anyone can do. Why do some managers think this train of thought? They think this train of thought because very few challenge the idea (managers, developers, and customers alike). Software projects get complete, one way or the other and code quality gets put on the back burner. Now, if projects are being completed and they are being sold and accepted by customers, then why should a manager care about code quality?

The reason to care about code quality is productivity. As a developer who has taken over various code bases of shit code, I have run into the following problems (as a note for nomenclature, I refer to Java):

* Improper package usage - Packages, just like file system directories exist for a reason. They offer an internal structure for organizing relating code. Packages, just like databases, can be normalized, to organize code in a more specific and intuitive fashion.

Example: In a recent web development project, there was a single package for all servlets/action classes: com.project.action. There were nearly 100 servlets in the same class. No, these were not all related. They could have been organized more neatly into additional sub-packages. This doesn't even address the issue of having 100 servlets. I'll get to that later.

* Convenience methods - Convenience methods can be used for great advantage. Most of time, they are completely unnecessary. Code each class as an individual interface. An API, if you will. If a single generic method can be created to meet the interface requirements, then create a single method.

Example: getDevice(), getDevice(String), getDevice(String[]). getDevice calls getDevice(String[]) internally passing an empty array of strings. getDevice(String) calls getDevice(String[]) internally passing a array of strings with length 1. getDevice(String[]) has the actual implementation. To not confuse the calling interface, it may be simpler to just stick with one method: getDevice(String[]). This covers all cases. Again, this doesn't cover the issue of passing a String. I'll get to that later.

* Code duplication - function() { { /* code block to do one thing */ } { /* code block to do second thing */ }; The two internal code blocks are reused in this method along with 100 others. Imagine if another method needs to also use the two code blocks. Yes, then that method will also have the two code blocks. Ok, maybe that's not so bad. How about if a bug occurs in one of the code blocks and it needs to be fixed? Yes, then all 101 methods will need to be revised to fix the bug. That's bad. REFACTOR. Move the code blocks into their own methods so if a bug is to occur, then it can be fixed in one place, rather than 101 places.

Example: The previous web project I mentioned has 100+ action classes/servlets. There are groups of them that have very similar functionality. Refactor! Refactoring duplicate code will allow for easier maintainance of existing code, plus making the lives of future developers more easy when they have to add new similar features! Now, fixing code duplication doesn't always have to be resolved with code factoring. There's another possibility - object oriented programming. Use it. Love it.

* Strong typing - Java has an object oriented structure to provide custom-crafting data types. A developer can roll their own from scratch or extend from an existing code base, either the core APIs that Sun provides, their own previous code base, or third-party APIs. On top of that, Java 1.5 (Java 5.0 - I hate marketing) introduced enums. Enums are short for enumerations. They are fantastic and should be used much more often than they are in practice. Yes, but why can't a developer use some of the standard APIs that I just mentioned above. For example, it is common to pass parameters as Strings or Integers.

The reason this is bad coding practice is that, rarely, a complex parameter can be most strictly defined as a String or Integer. A String or Integer is an object wrapper for internal primitive types that more procedural C programmers would be familiar with (char[] - an array of characters otherwise known as a string and int - a primitive integer). There are additional primitive wrappers (Long, Double, Float, Short, etc.) available as well. It sometimes makes good sense to use these simple wrappers. For example, if a parameter can only be most strictly defined as an arbitrary array of characters, then perhaps it makes sense to use String.

What if the arbitrary array of characters needs to be modified? Then it may not be as good of a choice since the primitive wrapper objects are immutable (they can't be modified after constructed). This may be an added benefit if you don't want to allow object mutability. It's considered good practice in many ways to restrict the mutability of your own custom-made classes.

Now, what if the string isn't arbitrary? For example, consider the method assignTransmissionType(String type). This works, but isn't as strict as it could be. It would make sense to make an enumeration of supported transmission types (Automatic, Manual) and more strongly type the parameter -> assignTransmissionType(TransmissionType transmissionType). This makes calling interfaces more intuitive, allows for easier extensibility, and has the great benefit of not allowing calling interfaces to utilize your API in ways that you do not support.

I have more, but I've run out of steam.



No comments: