The following post is part of series called ’Flowers.’ You can find a previous entry here: Flower 1 Wet Locators

This time we start with this masterpiece of an if statement.

Example From Hell

[csharp]
private IWebDriver webDriver;
public IWebElement LoggedUserName
{
get { return webDriver.FindElement(By.Id("someTableId")); }
}

public bool IsLoggedUserNameDisplayed()
{
bool elemeentFound = false;
if (LoggedUserName.Displayed)
elemeentFound = true;
return elemeentFound;
}
[/csharp]

To make it clear Displayed is a boolean property of IWebElement 

To be honest, I don’t know even where to start… When I first saw this Flower I  had to go for a walk to calm down.

So let’s start with the easiest with what we already talked about in the previous post.  Let’s fix encapsulation.

[csharp]
private IWebDriver webDriver;
private IWebElement LoggedUserName
{
get { return webDriver.FindElement(By.Id("someTableId")); }
}

public bool IsLoggedUserNameDisplayed()
{
bool elemeentFound = false;
if (LoggedUserName.Displayed)
elemeentFound = true;
return elemeentFound;
}
[/csharp]

This is an easy fix, but for the sake of explanation, let’s go step by step.

We’re going to start with eleementFound, that variable does nothing! It looks like it is doing something but takes a look at execution paths:

• Assign false to it and then return value.
• Assign false, then overwrite it with true and after return value.

It is obscuring the view. Let do straight return of constants. [expand=„Click for more info”] For now let’s no go into naming discussion cause eleementFound is the wrong name.- and I am not talking about a typo.[/expand]

[csharp]
private IWebDriver webDriver;
private IWebElement LoggedUserName
{
get { return webDriver.FindElement(By.Id("someTableId")); }
}

public bool IsLoggedUserNameDisplayed()
{
if (LoggedUserName.Displayed)
retrun true;
return fasle;
}
[/csharp]

Ok, Now this code is just wrong. By few small fixes, we have uncovered biggest issue of this methods. It’s complete redundancy If true return true.

[csharp]
private IWebDriver webDriver;
private IWebElement LoggedUserName
{
get { return webDriver.FindElement(By.Id("someTableId")); }
}

public bool IsLoggedUserNameDisplayed()
{
retrun LoggedUserName.Displayed;
}
[/csharp]

Why it is bad

So we had four lines of code. (well there is option with else, but I haven’t seen it yet) And it is doing exactly nothing. It was wrapping existing property in logic that was doing the same stuff. One of my professors used to say. Do not invent the Wheel a new.  And this code was doing that.

And so you would know – It is not a fake example. I have seen this in one solution five times – I am sure they were an effect of copy-paste coding.

And again we came to the same conclusion is the same as in the previous post. Redundancy and obscuring other more serious issues. It is another cut another small bleeding wound. This code is not dangerous alone, but it is a sign of rot.

Now let see something similar, yet a little different:

Case 2.0 GetLoggedUserName

[csharp]
private IWebDriver webDriver;
private IWebElement LoggedUserName
{
get { return webDriver.FindElement(By.Id("someTableId")); }
}

public bool IsLoggedUserNameDisplayed()
{
retrun LoggedUserName.Displayed;
}

public string GetLoggedUserName()
{
string loggedUserName = null;
if (IsLoggedUserNameDisplayed())
loggedUserName = LoggedUserName.Text;
return loggedUserName;
}
[/csharp]

Before we start there is question should this string be null or should it be string.empty? Without looking at the context of this method – no idea. But null usually means invalid value. So, for now, we leave it be. But we will get back to it later.

But again it is variable itself is pointless. You know the procedure, so we are going to skip few steps. Again for now leaving out naming issue

[csharp]
public string GetLoggedUserName()
{
if (IsLoggedUserNameDisplayed())
return LoggedUserName.Text;
return null;
}
[/csharp]

C# has nice operator ?:  it was created for this kind of methods.

[csharp]
public string GetLoggedUserName()
{
return IsLoggedUserNameDisplayed() ? loggedUserName.Text : null;
}
[/csharp]

Much better but to be honest we fixed a problem of code, but not the root of the problem. Let’s think about the goal of this method.

It returns logged user name, but if a name is not displayed returns null…  I find it strange… But we will analyze it in next Flower number 3  „Who needs SRP anyway?”