1

While working on existing code base, I usually come across procedures that contain Abusive use of IF and Switch statements. The procedures consist of overwhelming code, which I think require re-factoring badly. The situation gets worse when I identify that some of these are recursive as well.

But this is always a matter of debate as the code is working fine and no one wants to wake up the dragon. But, everyone accepts it is very expensive code to manage. I am wondering if are any recommendations to determine if a particular Method is a culprit and needs a revisit/rewrite , so that it can broken down or polymophized in an effective manner. Are there any Metrics (like no. of lines in procedure) that can be used to identify such segment of code. The checklist or advice to convince everyone, will be great!

Sumeet
  • 121
  • 1
  • 6
  • possible duplicate of How would you know if you've written readable and easily maintainable code? – gnat Jun 29 '12 at 12:18
  • 1
    Why does everyone hate if statements? they are required in programming. If the code works why are you going to change it, furthermore, there are not a great deal of altneratives to if statements. – Ramhound Jun 29 '12 at 12:24
  • 2
    @Ramhound the key term is abusive, if you have an if/elseif/else block that is 20,000 lines; that is a huge statement machine that isn't going to be able to be easily reasoned about ( same with a switch ). There are some great alternatives such as the Chain of Responsibility pattern or the Strategy Pattern to deal with unwieldy code like this. –  Jun 29 '12 at 12:50
  • 1
    @Ramhound: IMHO, already 200 lines are far too many in one if/else statement, there are most times too many in one method. – Doc Brown Jun 29 '12 at 13:23
  • 2
    @Ramhound: we hate if statements because they increase cyclomatic complexity. Each if statement doubles the number of paths and the number of test cases. – kevin cline Jun 29 '12 at 15:27

7 Answers7

7

If you want to change this code, first write unit tests that validate it's functionality. Then as you make changes you will have confidence that you haven't broken anything.

There is no hard and fast rule about numbers of lines, but a function with 40+ lines is certainly a hint that it is too long. A function with 2 lines is not too small. Steve McConnell's Code Complete 2 refers to some studies and says that 200 lines is the upper limit for readability.

It's all about cohesion. A single function should do a single task, and all parts of the function should relate directly to performing that task.

Consider cyclomatic complexity. A high number of decision points and deep nesting may indicate too much code in one function.

Also, well-selected function and parameter names make code more readable because they act as a form of documentation. Everything that a function does should be immediately obvious from reading its name and its parameters. If it's hard to explain what a function does in a few comment lines, then you probably have too much code in one function.

sourcenouveau
  • 6,490
1

I have a simple guideline which I read somewhere maybe ten years ago. I like it when a method definition fits my screen, I don't like scrolling to read.

I do believe striving for shorter methods is good, makes you question whether you can achieve the same functionality in less code - thus less bugs. Also, in my experience when a method is too long, it is probably doing too much.

Selim
  • 126
  • 3
0

In my opinion, you cannot generalize how many lines of code a method or class should contain before breaking it up in smaller components. Instead of counting number of lines, you should count how many times a number of lines are reused. In example, if you have 3 lines of code you reuse again and again inside a method or class, then it is a good idea to extract this code into a method.

Another important aspect is your business domain. If you begin to break up your classes, just because you think there is to many lines of code, you will probably have to generate new classes and methods, with new names, that not necessarily fits the business domain. This kind of code is very hard to read and understand for other developers (and yourself after a few months).

If you feel that some code are unnecessarily complex, then you should consider rewriting it instead of breaking down the code. When analyzing and rewriting, you may find that it is a good idea to break some of the code down, but I don't think that should be the main objective. Over complex code will still be over complex, even if it is broken down.

gnat
  • 21,213
  • 29
  • 113
  • 291
aaberg
  • 101
  • 2
0

IMHO you should break up your code, so it only does one thing (not counting the argument checks). In general, If statements should be viewed as opportunities to extract functionality :-) In effect, the if method makes the code do two things.

Now, when to do it is another question. Do it quietly whenever you have the slightest interaction with it -- i.e. implementing new functionality or bugfixing. If someone asks, you did it to make sure, that your new code works properly -- and that would not even be lying

Morten
  • 903
0

I prefer to give my functions "speaking names", so I can save on commenting stuff. This leads to many small functions, which do specific operations.

So I extract a function from code, if...

...I use the code at more places.

...the name of a function would get rather long and implying many operations of different concerns.

With this approach I never end up with a function of more than 50 lines and readable code.

K..
  • 606
  • Those little functions get reused more than one might expect. I have found that short functions = shorter code. – kevin cline Jun 29 '12 at 15:28
0

It's a very easy rule: Functions should be short. As in, 5-7 lines.

It's almost always worthwhile to refactor very large functions, but you do need unit tests first.

Note: It's much easier to refactor large functions before they become part of the accepted codebase, and the way to do that is code review. I.e. when someone else codes up some monstrosity, you BLOCK the commit at the code review and make them fix it.

Kevin
  • 1,341
-2

I don't think that there are a specific limit on number of lines that a procedure can contain. However, it should never be on a developer's mind.

There are a couple of things that one can apply to ensure scaling, manageability and readible to new developers.

  1. Always try to do as much processing at the backend (database) side where possible when it comes to logic. Let your stored procedures handle as much of the bulk of the logic as possible to release the frontend code of unnecessary processing (IF,switch statements) as it might have a performance cost when multiple threads are running to take care of. This is of course if the logic make use of dataset from a database.

  2. A method should always have a single responsibility. That means that if your method
    calculates interest then it should not be responsible for emailing the user/client the daily interest as well or a method where you add you items in a basket must not
    handle the payments for you as well. It must have one purpose.

  3. A method should always be open for extention and closed for modification. Modifying the method changes it's purpose and complexity. Extending it keeps purpose and makes it more powerful.

  4. Rather have multiple classes seperating work than one class doing all the work. This removes unnecessary complexity. This is called Interface segregation.

Point 2,3,4 are the S, O and I of the SOLID principals ensuring that code are scalable, managable and also readible when new developers join the team.

To verify your methods complexity run Visual Studios Code metrics (http://msdn.microsoft.com/en-us/library/bb385914.aspx). If the Maintainability Index of a class/method is smaller than 75% then it warns you of a method becoming to complex to handle. There are various other indicators using Code Metrics.

JLC007
  • 147
  • 3
    I disagree on your first point: Database should hold data, period. – Louis Kottmann Jun 29 '12 at 14:39
  • I cannot imageine that one will use something like linq-to-sql or even large if/switch statements (codebehind) from only one thread to pull a million plus records from the database where a Stored Procedure can handle the processing situated increasingly powered up database servers. what about 10 threads? – JLC007 Jul 02 '12 at 08:49
  • That's exactly what you didn't understand: you should make sprocs that take narrowing parameters, so that you can do queries that return a low amount of data WITHOUT putting logic into the sprocs. The server code does the calculations for those parameters. One of the main reasons for this is that you can't debug SQL properly. – Louis Kottmann Jul 02 '12 at 09:29