Showing posts with label Code Smell. Show all posts
Showing posts with label Code Smell. Show all posts

Friday, June 15, 2012

Long Parameter List: A very long function declaration

Todays topic of discussion is long parameter list, to be sure that we are in same page I must first mention that “long parameter list” is a kind of “code smell”. Wikipedia has nothing to say about it, if anyone get a reference of a good resource please feel free to add in the comment section.

What is long parameter list?

Method or procedure that take too many argument tend to produce complex code and unreadable complex method, developer need to dive big time in a method to find out what it is doing!

But as big things has small beginning no method has this smell at the beginning I guess, as time progresses and developer start putting parameter one after another to accommodate long parameter list in a function and some thing end up with not only long parameter list but also long method.

Bellow a long parameter list method is given!

public SomeDetail GetSomeDetail(string payingId, string receivingId, string paymentStream, double holdingPercentage, int holdingPeriod,int param, int param2, int anotherparam, string moreparam, string andmore)
{
---------------------your code goes here
}


Where Long parameter list is allowed?


In some language there is possibilities of adding default value to a parameter, for instance C++, Python and C# (.NET 4.0 onwards) have this ability to have default value for a parameter on the other hand C and Java do not have such ability.


How to avoid long parameter list?


First of all the most easy way out is to use object class not clear ? for instance we have a long parameter method which is having a1,a2 …. a*n parameter, we can form a class named “ourclass” and then add members like “a1,a2…. a*n” and then pass single object to a method.


Secondly we can break the method in few smaller piece of method so that each method have few parameter and do only one thing, as long parameter list methods tend to do lot of work all by it self.


How lets look at a good example!



public IEnumerable<RepositoryItemInfo> GetAllDocumentInfo(string directory, string[] filteredExtensions)
{
return Service
.GetAllDocumentInfo(GetProxyCredential(), directory, filteredExtensions)
.Select(proxyDoc => ConvertProxyDoc(proxyDoc));
}


private static RepositoryItemInfo ConvertProxyDoc(RepositoryItemInfo info)
{
return new RepositoryItemInfo
{
Created = info.Created,
FullName = info.FullName,
Id = info.Id,
IsFolder = info.IsFolder,
LastModified = info.LastModified,
MetaData = info.MetaData,
ModifiedBy = info.ModifiedBy,
Name = info.Name,
FileSize = info.FileSize
};
}


In above code we can see that RepositoryItemInfo is a class which warps 9 parameter in a simple class and is been passed to a service method to get information.


References


Friday, June 8, 2012

Speculative Generality: Build something even if you don’t need it right now!

Today we will have a short discussion about Speculative generality, it yet another kind of code smell, not in case of actual code but in case of design.

Did you ever write code that was not needed right then , rather you wrote it in case you needed it in future?, If the answer is yes then you probably, in fact did introduced Speculative generality with or without knowing it.

Here is advice from the Experts

“Please don’t write anything in your code base unless your needed it!”

Lets be frank we don’t know what client want, infect the preceding statement is not true for all cases for instance when you follow strict water fall, but that is a whole different store as far as software development processes is concern. So in an ideal world I would say that on 80% client don’t know what they want so its obvious that developers can not predict, when we build something and show it to customers, customers will always will have some opinion on average case they want some modification.

So we have to build just what is necessary for reaching a short goal, after that along the way we would start fit whatever is necessary.

An Interesting Example could be, event though we don’t need any Abstract class for a view we spontaneously created an interface and an abstract class.

We can put down lots of stuff like that.

Rules

  1. If there is only one class that implements a interface probably we don’t need it.
  2. If there is only one sub class of a superclass, again probably we don’t need it.
  3. If only single function works okay for a method we don’t need any overload.
  4. And so on.

So from now on we would avoid any code that is not necessary.

Friday, June 1, 2012

Switch Statement: Same choice once again.

You have been there and done that thousands of time, but did you know that switch statement is a code smell? I am sure lot of people don’t know about this. I my self got surprise to know that switch statement is a code smell, when I attended my first agile process training.

Why switch statement is code smell?

Where ever there is a need for switch statement there is a good chance of polymorphism. But we must need to consider the context as well. Note that there is another form of switch statement,

(if … else if … else if .. )  is also another from of switch statement where polymorphism can be considered.

One thing we must consider that where converting a switch statement to a polymorphic solution often create a bunch of classes so make sure before creating classes that they worth doing and actually abstract some part of the code and take significant amount of code in a child class so that it does not become a lazy class.

Friday, May 25, 2012

Primitive Obsession:I will design all from scratch!

This is one of the primary tendency of every fresher developer or programmer, I my self is not out of the boundary, After starting the programming I was also kind of had the same tendency, I didn’t have much idea about the vast class library and functionality the c++, java or c# has to offer, rather I used the basic and primitive data structure and ideas and techniques that I learned while coding c.

This simple ignorance is some time called “Primitive Obsession”.

Here is what “codinghorror” has to say about “Primitive Obsession”,

“Don't use a gaggle of primitive data type variables as a poor man's substitute for a class. If your data type is sufficiently complex, write a class to represent it. ”

Code that has primitive obsession has the following phenomena 

  • use of primitives data type (like integers or strings) for solving complex problem.
  • use of low-level methods to perform operation on data.

Eventually we loose a higher level of abstraction.

One simple example in c# could be build a list of objects, we could build a class to keep a specific type of data and then expose different methods and properties to support the class,

Or

We can use a List<T> to keep the object, where what ever we need from a list is there. I think you got the idea.

References

Friday, May 18, 2012

Oddball Solution: some one does the same thing in different way.

In this section we would take a look at oddball solution code smell. So what is a odd ball solution, if one problem is solved in one way throughout a system and the same problem is solved in another way in the same system in some cases, one of the solutions is oddball solution.

This particular smell is also known as Inconsistent Solution.

This happens specially in case of algorithms, different algorithm or different version of the same algorithm is been used several places which creates inconsistency and duplicate code.

How odd ball solution get in to the system?

There are two obvious reason for this,

  • Ignorance of how a solution is implemented elsewhere in a system.
  • Not spending enough time refactoring code to use a consistent solution.

 

How to get rid of odd ball solution?

The Simple process to get rid of oddball solution is to use extract method and use same method and use same algorithm all over the system.

While picking the right solutions for a problem we must consider which solution is been used majority of time then decide if this solution is better than the one is been used minority times. Compare and then keep the best one and eliminate the other one.

Thursday, May 10, 2012

Lazy Class : Does not have any purpose in the Matrix.

There is a very nice saying in the movie matrix, every thing is the matrix has purpose, other wise its been deleted by the agents. Believe we also create purpose less classes in our projects. as we all can guess the purpose of this discussion is to take a small note on “Lazy Class”

Here is what Wikipedia has to say about Lazy Class.

“A class that does too little”

This particular code smell is also known as freeloader. A Lazy class does not start from the beginning, And had definite purpose, but after some move method and refactor the class gets so smaller in size and the minor functionality that it has can be offered by another more meaning full class, or perhaps it already offered by some one other class already. So it end up with doing nothing at all.

So we don’t feel pity about it and can delete the lazy class.

How to eliminate Lazy Class?

Use “Collapse Hierarchy” or “Inline Class” to eliminate this code smell.

Thursday, May 3, 2012

Large Class : The making of code smell history.

In this particular short discussion we are going to take a good look at “Large Class” code smell, and some obvious reason for creating one in any project. And then we would discuss some easy way to eliminate the large class code smells.

“Large class: a class that has grown too large”

This is what Wikipedia has to say about large class. And the statement is so true, as no class is a large class at the beginning, but as time passes some how it started to grow big and big and eventually end up with a unmanageable situation.

Who a large class?

  1. A class do too many work
  2. A class has too many methods and members both private and public and they used for versatile purpose.

In a large class we tend to add more functionally, when we are unsure where to put them, in this way more and more unsure stuff gets added and eventually got spoiled. Just like large method large class is very hard to manage and understand, a class has too many responsibility and do too many work all by it self, where as it could be separated in few smaller classes or perhaps could be refactored in such a way so that they do violate “Single responsibility principle”.

How to eliminate large class?

First of all we have to separate the responsibility and identify the set which belong to one group, and create another class to which has one single responsibility and using “move method” refactor technique move the related field and method to another class.

Follow the above process until all the responsibility is been distributed to other class.

Friday, April 6, 2012

“Long method” is one of the most common code smell!

You wont find a single project where long method code smell is not present. I find often this code smell often in my projects, and developers do not deliberately introduce long methods, it just got introduced along the way when we introduce features.

What happen when you find a long method?

  • You wonder why is this method is so long.
  • You try to understand what the code is doing.
  • You start hate the software because its full of lines after line but no end of the method.
  • You lost the track where you ware before.
  • And so on.

You can list hundreds of line like the above listing about long method, the story will go on and on. here is what “Wikipedia” has to say about the long method,

Long method: a method, function, or procedure that has grown too large.”

Bellow an example of a long method is given

void SolutionLinkBrowserNavigating(object sender, WebBrowserNavigatingEventArgs e)
{
var pageName = e.Url.ToString().Split('/').LastOrDefault();
if (IsLinkToTmeTool(pageName))
{
if (pageName != null)
{
var toolName = Regex.Replace(pageName, TMEToolPrefixPattern, "", RegexOptions.IgnoreCase);
e.Cancel = true;
if (PackageLoader.Instance.IsToolAvaiableForCurrentUser(toolName))
{
ToolManager.Value.ActivateToolCommand.Execute(toolName);
}
else
{
new Lazy<IMessageBoxPresenter>().Value.Show(
StringTable.ToolNotAvailableErrorMessageText, StringTable.ToolNotAvailableErrorMessageTitle, DialogButton.OK);
return;
}
}
}
else
{
var queryString = e.Url.Query;
const string pattern = @"^\?.*POwner=(?<Owner>[^&]+)";
var match = Regex.Match(queryString, pattern, RegexOptions.IgnoreCase);
if (match.Success)
{
var ownerPackageId = match.Groups["Owner"].Value;
var allowedPackageIds = PackageLoader.Instance.GetAllowedPackageIds().ToList();
if (allowedPackageIds.Contains(ownerPackageId) == false)
{
e.Cancel = true;
new Lazy<IMessageBoxPresenter>().Value.Show( StringTable.PageNotAvailableErrorMessageText, StringTable.PageNotAvailableErrorMessageTitle, DialogButton.OK);
return;
}
}
}
}



Bellow a edited version of the code is given. where the code is refactored with extract method and looks more understandable, even though you can refactor the code to a much simpler version, but this is given there as an example.


void SolutionLinkBrowserNavigating(object sender, WebBrowserNavigatingEventArgs e)
{
var pageName = e.Url.ToString().Split('/').LastOrDefault();
if (IsLinkToTmeTool(pageName))
LoadLinkedTool(e, pageName);
else
HandleOtherLink(e);
}

private static void HandleOtherLink(WebBrowserNavigatingEventArgs e)
{
var queryString = e.Url.Query;
const string pattern = @"^\?.*POwner=(?<Owner>[^&]+)";
var match = Regex.Match(queryString, pattern, RegexOptions.IgnoreCase);
if (match.Success == false)
return;
var ownerPackageId = match.Groups["Owner"].Value;
var allowedPackageIds = PackageLoader.Instance.GetAllowedPackageIds().ToList();
if (allowedPackageIds.Contains(ownerPackageId) == false)
{
e.Cancel = true;
DisplayMessage(StringTable.PageNotAvailableErrorMessageText, StringTable.PageNotAvailableErrorMessageTitle);
}
}

private static void DisplayMessage(string message, string title)
{
var messageBoxPresenter = new Lazy<IMessageBoxPresenter>();
messageBoxPresenter.Value.Show(message,title, DialogButton.OK);
}

private void LoadLinkedTool(WebBrowserNavigatingEventArgs e, string pageName)
{
if (pageName != null)
{
var toolName = Regex.Replace(pageName, TMEToolPrefixPattern, "", RegexOptions.IgnoreCase);
e.Cancel = true;
if (PackageLoader.Instance.IsToolAvaiableForCurrentUser(toolName))
ToolManager.Value.ActivateToolCommand.Execute(toolName);
else
DisplayMessage(StringTable.ToolNotAvailableErrorMessageText,StringTable.ToolNotAvailableErrorMessageTitle);
}
}


There are lot of refactoring tips and techniques to handle long method, and this is just the tip of the iceberg, this is just an example you can do many things and there are huge possibilities. Until next time me Md. Masudur Rahman Signning out.

Friday, March 30, 2012

Comment: Yet another code smell

Today we are going to take a look, and discuss on yet another code smell named “Comment”. Sounds like some thing is been told wrong, yes you guys heard right, comment is one kind of smell. The days of well documented code with comment are gone, now time has changed and new agile wind is flowing, as a result comments are now considered as code smell.

How Comment is smell?

Who would have thought that this day will ever come, I have worked with organizations where the developers is been given extra time to put right comment on code, but its history. How comment is smell? its simple the code is not self explanatory, so the code is smelly.

“The idea is to make each and every piece of code self explanatory.”

If your code need comment, you should take careful consideration to refactor your code so that it need no comment at all.

Some Useful Example

In bellow code what ever comment you are seeing is a comment smell.

/// <summary>
/// this method gets a tag or a country
/// </summary>
/// <param name="countryName"></param>
/// <returns></returns>
public static string GetTag(string countryName)
{
//loop through the dictionary key
foreach (string tag in _dictionary.Keys)
{
//search the key
List<string> patternList = _dictionary[tag];
bool found = false;

//if the tag is found return it
found = IsFound(countryName, null, patternList);

if (found)
{
return tag;
}
}
return string.Empty;
}
/// <summary>
///
/// </summary>
/// <param name="title"></param>
/// <param name="description"></param>
/// <param name="patternList"></param>
/// <returns></returns>
private static bool IsFound(string title, string description,
IEnumerable<string> patternList)
{
//traverse in pattern list
foreach (string pattern in patternList)
{
//get rid of . and /.
string searchPattern = pattern.Replace(".", "/.");
string sourceContent = title;
//use regex to find the tag match
bool found = Regex.IsMatch(sourceContent, searchPattern, RegexOptions.IgnoreCase);

if (!found && !string.IsNullOrWhiteSpace(description))
{
sourceContent = description;
found = Regex.IsMatch(sourceContent, searchPattern);
}
if (found)
{
return true;
}
}

return false;
}



Which comment is Allowed?



  • In a function a algorithm is been implemented, for instance a tax calculation procedure, and the procedure is been implemented following a particular rule in a tax book. If you provide the name and reference of the book its not a comment.
  • Class responsibility declaration, its okay to put few lines of comment above a class, and describe its purpose.

Its all about judgment


In the end a programmers judgment matters, we would know when to comment and where not to comment, with little refactor effort we can avoid comment.

Thursday, March 8, 2012

Duplicated Code: The evil twin sister of you favorite function.

Duplicate code is one of the most common code smell we have in our codebase. Its simply we don’t care that much about reusing, we know how to reuse objects, functions, even solutions or component but we are some what careless about removing duplicate code. You would be surprised to know that what amount of duplicated code exists and what are those? Off course there are some obvious techniques to eliminate those.

Here is what Wikipedia has to say about it",

“Duplicate code is a computer programming term for a sequence of source code that occurs more than once, either within a program or across different programs owned or maintained by the same entity. Duplicate code is generally considered undesirable for a number of reasons.[1] A minimum requirement is usually applied to the quantity of code that must appear in a sequence for it to be considered duplicate rather than coincidentally similar. Sequences of duplicate code are sometimes known as code clones or just clones.”

There are two type’s of duplicate code
  1. Blatant –same code appears in more than one places (Example: two methods, two related classes, two unrelated classes, etc.)
  2. Subtle-looks like different code but serves the same purpose
The following are some examples of duplicate code.
  • character-for-character identical
  • character-for-character identical with white space characters and comments being ignored
  • token-for-token identical
  • token-for-token identical with occasional variation (i.e., insertion/deletion/modification of tokens)
  • functionally identical
How duplicate code find the way into our code base?

The process is more than simple. you knowingly introduce duplicate code, and the possible candidate is

  • copy and pest code and use.
  • write similar code (subtle duplication).
  • rather than using library re-inventing the wheel.

There could be thousands of example that can be put down here, but would not be appropriate, but we have the idea, lets go and find duplicate code and eliminate them, it’s a game and we have to play it with huge interest.

Summery

The bottom line is that we must eliminate duplicate codes, while doing coding just give a breather to your mind and try to find out if there is already a code there or not that do the same purpose, and could it be refactored in such a way so that it could be used by your case also? Most importantly while working with legacy code we need to be more careful and keep our mind open to refactor a lot to eliminate duplicate codes.

References

http://en.wikipedia.org/wiki/Duplicate_code

Wednesday, March 7, 2012

Technical Debt: Avoid it at any cost, other wise it would take all your properties.

Here is a fact that I learned from a real life problem, on of my relative took a huge loan from a bank, the risk was totally calculated but its like he is been paying the interest and principle forever, and its a never ending story, Same thing happens to software if you do technical dept. Here is what wikipedia has to say about technical dept.

“Technical debt (also known as design debt or code debt) is a neologistic metaphor referring to the eventual consequences of poor software architecture and software development within a codebase. The debt can be thought of as work that needs to be done before a particular job can be considered complete. As a change is started on a codebase, there is often the need to make other coordinated changes at the same time in other parts of the codebase or documentation. The other required, but uncompleted changes, are considered debt that must be paid at some point in the future.”

In short, while doing coding we faced a problem and we do a quick and dirty way to solved it and give a on time delivery. Later you are coming back to the same place again and again to write more dirty code to over come the scenarios its failing.

What a stupid thing to do, if only your higher stakeholders known, that, the two hour job eventually will take huge amount of development time, they would have hired people who would be monitoring so that no technical debt is been introduced in the code base.

if you want to know more about this topic please go to http://en.wikipedia.org/wiki/Technical_debt. There is a significant amount of information is there to help you out.

Suggestions

What to do?

Never ever introduce technical debt.

If a problem takes good amount to time and a significant amount of architectural change talk to your higher authorities, make them understand that this would take time and a quick and dirty solution will lead them more development effort in future.

Take calculated risk

Always follow 80/20 principle if the feature is been used by only 20 % of the users and its unlikely that this would cause any immediate breakdown.  But don’t forget to get back to it quickly and then refactor it, other wise you would soon forget about it.

General Suggestion

  • Refactor as you go deep in to the code.
  • Try to use design pattern
  • Be communicative with the team
  • Always follow good process of development (For Example : Agile: Scrum, Kanban)
  • Most important try to have some test coverage.

In an ideal development world there is no technical debt, but still there is in real world. so we got handle it correctly, until next time happy coding.

Friday, February 17, 2012

“Dead code”, a very common code smell.

Remove dead code from you code files

Today we are going to take a look at a simple code smell named “Dead code”. You would be surprised to know how many dead code exists in your project when you take a close look at the project file. of course you wouldn’t know just by looking at it. some of them are pretty hard to find.

What is dead code?

The code that is no longer needed and no longer used by any function or procedure is a dead code.

The Tool

The easiest way to identify the dead code is to use a tool that will save thousands of milliseconds by providing some indication that the code is no longer used. And I am talking about none other than Resharper, the awesome tool that solve lot of our problem and yet so cheap!. Of course I am talking about only Microsoft Platform, eclipse has its owe set of cool feature that have all the refactor and code notation. But if you are a “IntelliJ IDEA” user, then probably you have all the necessary help you need.

Simple dead code

The bellow screen shot shows some portion of the code file and the red rectangles are dead code. that’s the simple version I would say. If you have Resharper plugin with your VS2010 IDE this tool would grayed out the dead to let user know what are the dead codes.

image

Here is possible things that can be dead code.

  • private variables
  • private methods
  • private properties
  • redundant quantifier
  • redundant directives
  • public methods
  • public properties
  • Even a class
  • Unused method parameter
  • Unused local variable

Remove dead code Techniques

#1 :  Use the full cleanup .

Just right click on your code file and select full cleanup from context menu. This would remove lots of problem from your code.

image

image

Yet you have some dead code after full clean up. you can follow the bar of VS code window and click imageon each item to remove the problems. to remove the dead field or method just put the cursor on the gray text and then hit key ctrl+Enter this will pop up a tool window with some option menu with it. Select Remove unused method to get rid of the method.

image

 

#2 : Find the uses of the method

This is a hard part and can lead to a chain where you would get rid of a couple of methods Tree. How do you quickly identify that the procedure is unused. Its pretty easy to identify the private methods and fields as its been grayed out by the reharper. but what about public method and public properties or fields.

Generally when you get rid of a dead code for instance a private method, any method that is been used in that private method also become dead if the method is not been used some where else, luckily reharper automatically detects it and gray out the methods in turn.

Now lets talk about public methods, first check if the method has a reference or not. To do that simply right click on a method and select “find usages” if it shows that it does not have any uses, you can probably get rid of the public method,field and property.

image

One easy way to use the modifier, change the modifier from public to private. if you don’t see any compile error it means this method would be grayed out. so no uses. But this fails in case of WCF Service interfaces. Since the service is been uses some where else and all methods that we want to expose needs to be public. But that is the whole idea of a WCF service, so we would expose only necessary methods That would be used.

After find Usages if any usages found it would be listed on a tool window. Just click on the item. It would take the user to the method or line of code where its been used. Check again if that method has any Usage. And so on , I think you got the idea what needs to be done here to identify where is the dead code.

image

A common problem of our code is that is the class is been used anywhere? One simple technique is simply exclude the class from the project and then build the project see if it creates any compile error or not. but we can also use find usage technique with the class file also, to find if its been used some where or not.

Always use “Safe Delete”

While removing class or methods from project always use safe delete operation of reharper, this would help us not to create unnecessary problem for our codes. To use safe delete simple right click on the object you want to delete, i.e. parameter, variable , method and class. Then select Safe Delete from context menu, or you can also use Ctrl+R, D to lunch the safe delete window.

image

When the window pop’s up you can also check enable undo if you don’t feel safe. Then click next to get rid of the code. If the code that we ware trying to delete has some reference some where it would not perform the operation rather it would show a error window showing where the usage is.

image

There are many more thing to discuss about dead code. And couldn’t be completed in a single post. I would try to put together different platform in future.