Refactoring 3

[whosrdaddy]

Hi,

I am a experienced delphi developer and I am attempting my first real c# (asp.net mvc) project.

this piece of code validates my website user.
I am sure it can be be refactored, but i'm not yet fully acquainted with the c# syntax. can someone help me with this?

 user = userRepository.GetByUserName(userName);

                if (user != null)
                {
                    if (user.UserName == null && user.UserName != userName)
                    {
                        validationMessage = "Invalid username";
                        return false;
                    }

                    if (user.Password != password)
                    {
                        validationMessage = "Invalid password";
                        return false;
                    }
                }
                else
                {
                    validationMessage = "Invalid username";
                    return false;
                }

thanks!
/Daddy


-----------------------------------------------------
What You See Is What You Get
Never underestimate tha powah of tha google!

 

[whosrdaddy]

here is the complete function:

 public bool ValidateUser(string userName, string password, out string validationMessage)
        {
            User user;

            try
            {
                user = userRepository.GetByUserName(userName);

                if (user != null)
                {
                    if (user.UserName == null && user.UserName != userName)
                    {
                        validationMessage = "Invalid username";
                        return false;
                    }

                    if (user.Password != password)
                    {
                        validationMessage = "Invalid password";
                        return false;
                    }
                }
                else
                {
                    validationMessage = "Invalid username";
                    return false;
                }

                validationMessage = "";
                return true;
            }
            catch (Exception ex)
            {
                throw new Exception("Encountered an error while attempting to validate username and password", ex);
            }
        }

/Daddy

-----------------------------------------------------
What You See Is What You Get
Never underestimate tha powah of tha google!

 

[jmeckley]

there is 1 glaring issue with exception handling, but other than that it looks OK.
the issue with the exception handling is that
1. you are catching the base Exception (usually you don't do this)
2. you are swallowing the actual exception (type, message, stacktrace) which you need to figure out what happened.

typically I avoid try/catch as much as possible and let the exception bubble up to application layer. at this point I log the exception and redirect the user to a generic error page. if the exception is that exceptional, I don't want the user to continue to do anything.

there are guidelines all over the net on .net exception handling if you want more details.

personally I avoid out/ref parameter operations so I would remove that, but it's not necessary. One option would be to return a validation response

public ValidationResponse ValidateUser(string userName, string password)
{
   if(not a valid user)
   {
      return new ValidationResponse("error message");
   }
   return new ValidationResponse();
}

...
public class ValidationResponse
{
   public ValidationResponse(params string[] errormessages)
   {
       ErrorMessages = errormessages
   }

   public string[] ErrroMessages {get;private set;}

   public bool IsValid()
   {
       return ErrorMessages.Length == 0;
   }
}

usually login/validation does not distinguish between invalid username and invalid password. by telling a user which is wrong you can help hackers narrow down there results.

one other practice is to limit if/else blocks. this is a general OOP concept though, it's not specific to .net. The more if/else (or case statements) your code base has the more logic code paths a workflow can travel. I believe the term is called ornithological code/workflow, or something like that. It's a code metric. for academic reasons I don't fully understand
this.

if(a true statement)
{
   return one thing;
}
return another thing;

is preferred over

object foo;
if(a true statement)
{
   foo = one thing;
}
else
{
   foo = another thing;
}
return foo;

all that said you could refactor to something like this

public ValidationResponse ValidateUser(string userName, string password)
{
	var user = userRepository.GetByUserNameAndPassword(userName, password);
	if (user == null)
	{
		return new ValidationResponse("Invalid username or password");
	}
	return new ValidationResponse();
}

that's just one option though.

Jason Meckley
Programmer
Specialty Bakers, Inc.

faq855-7190
faq732-7259

 

[whosrdaddy]

Hi jason,

this is what I was looking for .

In fact I found almost the same thing:

public UserValidationResult ValidateUser(string userName, string password)
        {
            User user = userRepository.GetByUserName(userName);
            if (null == user)
            {
                return new UserValidationResult().InvalidUserName();
            }

            if (user.Password != password)
            {
                return new UserValidationResult().InvalidPassword();
            }

            return new UserValidationResult().Success(user.Init());
        }

about the exception : if you look at the exception code block you will see that I throw a new exception with a more meaningfull errormessage (this is the only place where I use the exception message to display it on the website, all other exceptions will go to the standard exception page).

Thanks Jason (and have a *)

/Daddy

-----------------------------------------------------
What You See Is What You Get
Never underestimate tha powah of tha google!

 

[jmeckley]

if you look at the exception code block you will see that I throw a new exception with a more meaningfull errormessage (this is the only place where I use the exception message to display it on the website, all other exceptions will go to the standard exception page).

I missed the original exception was passed to the new exception. i thought before you where just throw a new exception without referencing the original.

Jason Meckley
Programmer
Specialty Bakers, Inc.

faq855-7190
faq732-7259

 

[DansDadUK]

>> I believe the term is called ornithological
>> code/workflow, or something like that

Jason

'ornithological' is associated with the study of birds.

I suspect (but don't know) that the term you were after may be 'orthogonal'.

Chris

p.s. Keep up your excellent work on assisting others with programming issues.

 

[jmeckley]

LOL (yes really) birds!

decided to dig up the context in which I heard about this concept. the term I was looking for was Cyclomatic Complexity. The lower the metric the simpler the code.

I was way off! Don't know how I got to ornithological, or orthogonal for that matter.

Jason Meckley
Programmer
Specialty Bakers, Inc.

faq855-7190
faq732-7259

 

[BobRodes]

<It's a code metric. for academic reasons I don't fully understand
this.

From an higher-level OOAD standpoint, it has to do with the concept of cohesion. A class that lacks cohesion tries to do too many things, because it has been conceptualized with insufficient granularity. Poor cohesion is characterized by excessive decision logic (which a CC metric will uncover). Time spent deciding what to do is time spent not doing anything, which is why poor cohesion equates to poor performance.

 

[PGO01]

Would this be preferred or not?

object foo;
foo = some_logical_statement ? one_thing : another_thing;
return foo;
[\code]

 

[jmeckley]

that's just syntax sugar. I wouldn't say it is or isn't preferred. what you want to try to avoid is lengthy if/then and swtich statements
for example
this would be considered low cohesion (poor design)

if(something)
{
   do one thing
}
else if(something else && this || just that)
{
  do another thing
}
else if(yet another thing && !that)
{
  do something completely different
}
else
{
   if nothing do this
}

//or

switch(value)
{
    case 1:
        do one thing;
    case 2:
        do another thing;
    case 3:
        do something completely different;
    default:
        if nothing do this;
}

there are OOP concepts and pattern which can reduce the need for if/else and switch. At some point they are needed because there is a decision to be made. for example we could refactor the if/else statement into strageties. something like

interface ISituation 
{
   public void Execute(MyObject obj);
}

class SituationOne : ISituation 
{
   public void Execute(MyObject obj)
   {
     if(obj does not the criteria for situation 1) return;
     do one thing
   }
}

class SituationTwo : ISituation 
{
   public void Execute(MyObject obj)
   {
     if(obj does not the criteria for situation 2) return;
     do another thing
   }
}

class SituationThree : ISituation 
{
   public void Execute(MyObject obj)
   {
     if(obj does not the criteria for situation 3) return;
     do something completely different
   }
}

class DefaultSituation : ISituation 
{
   public void Execute(MyObject obj)
   {
     always do this
   }
}

when can then put all these situations into an array and loop through each

var situations = new ISituation[]{
        new SituationOne(),
        new SituationTwo(),
        new SituationThree(),
        new DefaultSituation()
   };

var obj = new MyObject{
        //load all criteria which can be used by any of the situations
   };

foreach(var situation in situations)
{
      situation.Execute(obj);
}

this looks drastically different than the lengthy and complex if/then/else we had before. What have we gained?
1. each situation is encapsulated into it's own object. if a new situation arises we just add a new implementation of ISituation to our project and add an instance of that implementation to our array.
2. we could also test each situation in isolation from one another with a testing framework (nUnit, MbUnit, etc.)

this is just one, very simple, example of refactoring to increase cohesion.

Jason Meckley
Programmer
Specialty Bakers, Inc.

faq855-7190
faq732-7259

 

[BobRodes]

>strageties

Sounds like a combination of the words "strategy" and "tragedy".

 

[BobRodes]

All kidding aside, there is one thing. The concept of iterating through a foreach loop also implies decision logic, and a cc metric wouldn't find the second solution much different from the first. You'll note that each foreach loop has an if statement in it.

Consider this piece of pseudocode:

Class UnitOfWork1_1 : UnitOfWork
Class UnitOfWork1_2 : UnitOfWork
Class UnitOfWork2_1 : UnitOfWork
Class UnitOfWork2_2 : UnitOfWork

Class DoAllWork //but only if needed!!
   put one of each UnitOfWork object in DoWorkCollection
   for each doWork in DoWorkCollection
       if doWork.Needed then
          doWork.DoStuff
       endif
   endfor
end class

Let's say an average of two units of work need to be performed in each instantiation of DoAllWork. This means that we are going to the extra effort of instantiating UnitOfWork objects that aren't going to be used, as well as checking them to see if they are going to be used or not.

Let's refactor using principles of cohesion, then. We do some research, and find that the first two classes are very often done together, and the next two are as well. We also find that the first two are rarely done with the second two, and vice versa. This pseudocode will produce the same capabilities as the first, but with finer granularity:

Class UnitOfWork1_1 : UnitOfWork
Class UnitOfWork1_2 : UnitOfWork
Class UnitOfWork2_1 : UnitOfWork
Class UnitOfWork2_2 : UnitOfWork

Class DoJob1
   put one of each UnitOfWork1 object in DoWorkCollection
   for each doWork in DoWorkCollection
       if doWork.Needed then
          doWork.DoStuff
       endif
   endfor
end class

Class DoJob2
   put one of each UnitOfWork2 object in DoWorkCollection
   for each doWork in DoWorkCollection
       if doWork.Needed then
          doWork.DoStuff
       endif
   endfor
end class

What we are doing here is decoupling the work in unit 1 from the work in unit 2, which are not closely related to one another. We are also decoupling the process (well, part of the process) of evaluating what is needed and assigning it to the consumer. So, consumers of the units of work provided may choose one or the other or both in accordance with what is needed and avoid going to the overhead expense of instantiating and evaluating what isn't.

Another point is that the consumer has the consumer's context to use to evaluate what the consumer needs, and the provider does not. As such, the consumer is likely to be able to do this in a more economical fashion than the provider. Thus, assigning to the consumer the process of determining what the consumer needs also represents an improvement in cohesion characteristics.

So, each unit of work is encapsulated with more closely related units of work than before, meaning that we have improved cohesion. As you can see, we have saved some performance overhead by doing this, demonstrating the value of good cohesion characteristics in architecture.

 

[jmeckley]

So your saying there is not a difference between if/then/else and looping through a set of commands? The reduction of CC comes from reducing the logical code paths? I assumed removing the "else" from "if" did that.

Jason Meckley
Programmer
Specialty Bakers, Inc.

faq855-7190
faq732-7259

 

[BobRodes]

I won't go so far as to say there is not a difference. I suspect that using foreach has less processor instructions than using an if/then/else block, for example.

<I assumed removing the "else" from "if" did that.
I'm sorry, but a quote from the link that you provide on CC suggests otherwise

Despite Indeed there is an intuitive way to compute CC : The CC value of a method is 1 +{the number of conditional statements contained in the method body}. A conditional statement is something very intuitive, in C# a conditional statement is a statement in the group…

if | while | for | foreach | case | default | continue | goto | && | || | catch | ternary operator ?: | ??


… while following statements are not counted for CC:

else | do | switch | try | using | throw | finally | return | object creation | method call | field access

Now, your decision logic does differ between the two approaches, because the behavior is different. In the first approach (I'm assuming you intended breaks in the switch, so it will behave the same as the if/then/else), conditions are evaluated only until one of them evaluates to true. In the second approach, all conditions are evaluated whether any of them evaluate to true or not.

If you make the behavior exactly the same, as here:

if (something)

else if (something else)

else

and

for each (object in a set of three objects)
   if (some method evaluates a condition and returns true)
       exit the loop
   end if
end for

You have more decision logic in the for each, because there is the added decision of whether to exit the loop after the internal decision has been evaluated.

So, it looks like the if/then/else approach, or the switch/break approach, is most efficient in a set of decisions where each decision's existence (except the first of course) depends on the outcome of a previous one. The iterative loop approach is probably more efficient (due to code optimization) when each decision is independent of the others, but not because there is less decision logic.

Now, I'd like to take up my understanding of cohesion. In the very interesting "Cohesion, Coupling and the Metatheory of Actions" (

http://www.scribd.com/doc/2680380/Cohesion-coupling-and-the-metatheory-of-actions),

there is the following explanation:

A module is cohesive when at the high level of abstraction it does only a single task. The more a module is focused on a precise goal the more it is cohesive.

A highly cohesive module will be simpler to understand, having to do only a single task, while a lowly cohesive module, performing so many tasks, will be difficult to understand.

Given this explanation of cohesion (which I agree with), excessive decision logic is an indicator of low cohesion, for it implies an excessive number of tasks to choose from. It therefore follows that reduction of tasks as indicated by reduction of decision logic increases cohesion.

By the way, taking reduction of decision logic to an extreme (typically, breaking a single task that has some decision logic into multiple related ones) creates the opposite problem of overcoupling. (Excessive communication between objects is an indicator of overcoupling.) So, a strong architecture balances the somewhat conflicting requirements of high cohesion and loose coupling, with the ideal of encapsulating and exposing single, well-defined tasks.

 

[BobRodes]

By the way, Jason, I don't mean to sound like I think mine is the last word on the subject. Please feel free to correct me if you think I'm wrong.

Bob

 

[jmeckley]

Bob, I think you have a better understanding of code metrics than I do. My exposure is reading other people's blogs. I haven't had a need to apply them to my projects.

for me, low cohesion translates to fewer "then" blocks and objects that are easier to test. I haven't needed to dive deep into refactoring on a level the the hypothetical scenario you described above.

thank you for providing your insight for the rest of us.

Jason Meckley
Programmer
Specialty Bakers, Inc.

faq855-7190
faq732-7259

 

[BobRodes]

Well, if you say so, then you are most welcome. As I've given myself to the goal of getting fully up to speed in C# by the end of the year, perhaps I will be asking people here for more help than I can return.