Avoid code Repetition in conditions

[xWhiplash]

Hello,

 

I have been programming for over 15 years now.  I consider myself a very good programmer, but I understand (like all of us) there are things that I need to work on.  One of these things is code repetition when dealing with conditions.  I will give a generic sample:

if(condition1)
{
     //perform some logic
     if(condition2)
     {
          //perform some logic
          if(condition3)
          {
               //Perform logic
          }
          else
          {
               //MethodA(param)
          }
     }
     else
     {
          //MethodA(param)
     }
}
else
{
     //MethodA()
}

Now, I cannot make it easy by doing the following:

if(condition1 && condition2)
{

}
else
{

}

I cannot do this since I need to perform some logic if condition1 is true and before I test condition2.

 

Is there a way to structure if...else blocks to where if you need to call a method in each else blocks, you are not repeating yourself?

[xWhiplash]

You can treat the Session_Start as an entry point function (It's an ASP callback function right?), so yes, you can do all kinds of things in it. However, if you do all of it in that one function, it will turn into a jumbled mess. So as I said earlier in this thread, split it into discrete functions, and give those tasks to them.

 

function main ()
{
     doWorkA();
     doWorkB();
     doWorkC();
    
     etc....
}

 

Yeah that is what I thought.  Thanks for all the advice!  I like to work on my weak areas in programming.

[simplezz]

I did not show the correct code on my initial post with the ASP.NET.  The actual code was

Account account = Account.GetUser(userID);

Which would return a null object if the userID was not valid.  Or I could go the constructor way and throw an exception.

So is this Account object some kind of factory class? Shouldn't it return a User object? Not that it really matters that much really. If you're happy with the code.

[xWhiplash]

So is this Account object some kind of factory class? Shouldn't it return a User object? Not that it really matters that much really. If you're happy with the code.

 

No it is just a class that holds the user data.  ID, First Name, Last Name, ...

 

Do you think it is better to call the constructor and have it throw an error?  Even if I just catch it just to log it in the database/event viewer?  I would be swallowing the exception at that point though.

[Andre S. Veteran]

Exceptions are expensive and supposed to represent exceptional states. If you fully expect to get invalid user ids then I'd (again) use the Try-Parse pattern:

class Account
{
    public static bool TryGetFromUserId(int userId, out Account account)
    {
        account = null;
        if (isValid(userId))
        {
             account = // valid account
             return true;
        }
        return false;
    }
}

[simplezz]

No it is just a class that holds the user data.  ID, First Name, Last Name, ...

 

Do you think it is better to call the constructor and have it throw an error?  Even if I just catch it just to log it in the database/event viewer?  I would be swallowing the exception at that point though.

If you posted the class, it might give us a better idea what its purpose is. It seems from the code you posted that each Account object must have a unique user id. If that's the case, then yes it would probably be better for the constructor to return some kind of error if that user id is already allocated. I'm not a C# expert though. Perhaps Andre can chime in whether or not throwing an exception would be the best way to communicate that error? Returning null from a constructor doesn't seem ideal.

[simplezz]

Exceptions are expensive and supposed to represent exceptional states. If you fully expect to get invalid user ids then I'd (again) use the Try-Parse pattern:

class Account
{
    public static bool TryGetFromUserId(int userId, out Account account)
    {
        account = null;
        if (isValid(userId))
        {
             account = // valid account
             return true;
        }
        return false;
    }
}

That would be one way to do it. So do these accounts already exist in some kind of collection (array/list/map)? Because one of the comments said

//Checks if the user was created successfully, and if they were able to login successfully

[xWhiplash]

That would be one way to do it. So do these accounts already exist in some kind of collection (array/list/map)? Because one of the comments said

 

The users are in a database.  The user ID value comes from the client (from the cookie), so I treat that as an invalid value until I test it.  As Andre said, exceptions are expensive.  If I test the value of the ID before I try to initialize the user object, and return null if it is negative for example, I can avoid some exceptions.  This is why I am asking.  Would it be better to validate the userID and return null if a user object does not exist, or deal with exceptions?

[simplezz]

The users are in a database.  The user ID value comes from the client (from the cookie), so I treat that as an invalid value until I test it.  As Andre said, exceptions are expensive.  If I test the value of the ID before I try to initialize the user object, and return null if it is negative for example, I can avoid some exceptions.  This is why I am asking.  Would it be better to validate the userID and return null if a user object does not exist, or deal with exceptions?

It sounds like Andre's solution would be better for your situation. In the article he linked to, it makes the case that throwing exceptions when a condition is expected and likely to happen, is prohibitively expensive.

You can receive the object in an out parameter if the boolean result is true. In this way, the code is much clearer, and you can avoid exceptions.

[xWhiplash]

It sounds like Andre's solution would be better for your situation. In the article he linked to, it makes the case that throwing exceptions when a condition is expected and likely to happen, is prohibitively expensive.

Your can receive the object in an out parameter if the boolean result is true. In this way, the code is much clearer, and you can avoid exceptions.

 

Would it be better to make a trip to the database to check if the user ID is valid?  Or would that be more expensive than an exception?

[Andre S. Veteran]

Would it be better to make a trip to the database to check if the user ID is valid?  Or would that be more expensive than an exception?

The trip to the database is likely much more expensive, but you should benchmark to be sure. I'm not sure I understand your dilemma though. Throwing an exception is not a way to check if the user ID is valid, it would be one possible way of reporting to callers of the method that the ID is invalid (a better way would be to use the Try-Parse pattern as I illustrated).

[xWhiplash]

The trip to the database is likely much more expensive, but you should benchmark to be sure. I'm not sure I understand your dilemma though. Throwing an exception is not a way to check if the user ID is valid, it would be one possible way of reporting to callers of the method that the ID is invalid (a better way would be to use the Try-Parse pattern as I illustrated).

 

Well how would I check to see if the user ID of 714653 is valid?  It is within the range of Int32 so it is a valid number for an ID.  I could at least check if it is negative.  But I would need to check the database if 714653 is a valid ID (a record exists).

 

If it does not exist, the Account constructor cannot do anything since it cannot get the username, first name, last name, ... from the database.  So maybe throwing an exception is the way to go?

[simplezz]

Would it be better to make a trip to the database to check if the user ID is valid?  Or would that be more expensive than an exception?

I thought that's what you were doing? Then creating the object on the fly with information extracted from the database record.

 

I mean, if you're doing an SQL select query anyway, in response to the GetUser() function, you'll know if that user id doesn't exist because they'll be no matching record. Unless I'm completely misunderstanding how your program is operating.

[simplezz]

If it does not exist, the Account constructor cannot do anything since it cannot get the username, first name, last name, ... from the database.  So maybe throwing an exception is the way to go?

Well, you could create a static factory function in your account class, such as:

class Account {
 
      public static bool TryGetUser ( int user_id, out Account acc ) {
                 /* SELECT FROM table where user_id = 'user_id' */ 
                 /* if query returns a record, the user exists */
                 if ( NULL == record )
                     return false;
 
                 /* now construct object from db record information */
                 Account a  = new Account ();
                 a.id       = record.user_id;
                 a.name     = record.name;
                 a.password = record.password;
                 ... etc
 
                 acc = a;
                 return true;
      }
      ....
}

As per Andre's Try pattern example.

[xWhiplash]

Well, you could create a static factory function in your account class, such as:

class Account {
 
      public static bool TryGetUser ( int user_id, out Account acc ) {
                 /* SELECT FROM table where user_id = 'user_id' */ 
                 /* if query returns a record, the user exists */
                 if ( NULL == record )
                     return false;
 
                 /* now construct object from db record information */
                 Account a  = new Account ();
                 a.id       = record.user_id;
                 a.name     = record.name;
                 a.password = record.password;
                 ... etc
 
                 acc = a;
                 return true;
      }
      ....
}

As per Andre's Try pattern example.

 

Yeah that works better than having to make two trips.  I always forget to use out parameters :)  Thanks!

[+theblazingangel MVC]

@OP

 

So I've only just now discovered the true purpose of the Session_Start function. As I previously stated, I'm not very familiar with ASP.net (though I have however written a fair bit of PHP in the past). Looking back you did actually mention it's purpose near the beginning of the thread, but I missed it. Up to this point I had assumed that the sole purpose of this function was checking the user's authentication, and that it was being called early on in the application, on every page load. Having found a little more time to go over the thread and think about things, I was actually starting to write a big post expressing some concerns I had with the overall design, but then for some reason I looked up and discovered the real purpose, leaving those concerns invalid and one big post deleted.

 

So, based on my new understanding of this Session_Start function's purpose, I'm most definitely in favour of suggesting that you move a lot of, if not all of, this 'persistent login' code out into one or more separate functions. Thus, if at any point you want to expand the Session_Start function with additional functionality, unrelated to persistent logins, you're in a good position to do so easily.

 

If you'd like to offer up a copy of all code to do with authentication, perhaps we'd be in a much better position to try to perhaps offer up some good suggestions on how we'd structure things.

[xWhiplash]

@OP

 

So I've only just now discovered the true purpose of the Session_Start function. As I previously stated, I'm not very familiar with ASP.net (though I have however written a fair bit of PHP in the past). Looking back you did actually mention it's purpose near the beginning of the thread, but I missed it. Up to this point I had assumed that the sole purpose of this function was checking the user's authentication, and that it was being called early on in the application, on every page load. Having found a little more time to go over the thread and think about things, I was actually starting to write a big post expressing some concerns I had with the overall design, but then for some reason I looked up and discovered the real purpose, leaving those concerns invalid and one big post deleted.

 

So, based on my new understanding of this Session_Start function's purpose, I'm most definitely in favour of suggesting that you move a lot of, if not all of, this 'persistent login' code out into one or more separate functions. Thus, if at any point you want to expand the Session_Start function with additional functionality, unrelated to persistent logins, you're in a good position to do so easily.

 

If you'd like to offer up a copy of all code to do with authentication, perhaps we'd be in a much better position to try to perhaps offer up some good suggestions on how we'd structure things.

 

I am going to change things around a bit to make it a bit better.  Yeah the code to create the user object will only appear twice, in the Global.asax file (cookie login) and in the Login.aspx file (username + password login).

 

So you think just have Session_Start handle the errors and just call a method is best?

[+theblazingangel MVC]

I am going to change things around a bit to make it a bit better.  Yeah the code to create the user object will only appear twice, in the Global.asax file (cookie login) and in the Login.aspx file (username + password login).

 

So you think just have Session_Start handle the errors and just call a method is best?

 

As others have said in this thread, it's about separation of concerns. The purpose of the Session_Start function is to initialise an empty session with all of the bits and pieces it needs. One of those things is a user object, for which your application requires code for login persistence to be run. The details of how exactly this login persistence code works; maybe even the fact that login persistence checking is part of setting up the user object stored in the session; and perhaps even whether a null gets stored instead of a user object when the user is not logged in, is none of the concern of this Session_Start function. All of that should be delegated out to an entirely different function, or set of functions.

 

In addition to authentication related code within global.asax and login.asax (which presumably just presents and processes the login form), you've also got some related code somewhere that runs on every page lode do you not? In your Session_Start code you're making a call to account.Login() which has a comment beside it mentioning that this returns false if a user has been banned, thus blocking login persistence from resuming their login. But unless you're doing a check on every page load, when you ban a user, that only prevents them from logging in again when they themselves log out, or resuming things when they let their session expire. While their session is still active, banning them has no effect. That is unless you're checking on every page load, or at least caching the result in the session and re-checking periodically (which would require an expiration check on every page load).

 

Furthermore, what if you implement a permissions system and you want to change the permissions of a user. Unless you fetch their permissions on every page load, the user's new permissions, whether you've enhanced them or reduced them, will only take effect if the user's session expires, or if they deliberately log out and back in again. Similarly other details cached in the user object, like username, may also present similar issues (if they can change their username or email, the old one being presented on pages after they changed it to a new one).

 

So if you're not fetching info about the user on every page load, perhaps you should revisit that. Assuming you have or add this additional code, where does this live?

 

I'd say that ideally, you probably want to extract this code from where it currently exists, group it all together in one place, and use the separation of concerns approach towards rewriting it, certainly removing any duplication. This may be a collection of functions within a single file, it may be a collection of methods with in a class, it may be spread across multiple classes and functions within a single file, it may be that it's best to split the code across multiple files, all are fine, but you should keep that file or those files specific to the task at hand. It may be that you have one file dedicated to all code for performing authentication, and a separate class in a separate file for user objects, which may hold properties and methods that don't have anything to do with authentication, hence why you've got it in a separate class and in a separate file. Then, the points in your program previously mentioned, the Session_Start event handler, the login form processing code, the block of code / function that executes on every page load to check ban/locked-out status and load permissions or whatever, as discussed above; each of these should utilise these authentication functions/classes to achieve their objectives, with the details of how it's done and the task of actually doing it being delegated and abstracted away.

[xWhiplash]

As others have said in this thread, it's about separation of concerns. The purpose of the Session_Start function is to initialise an empty session with all of the bits and pieces it needs. One of those things is a user object, for which your application requires code for login persistence to be run. The details of how exactly this login persistence code works; maybe even the fact that login persistence checking is part of setting up the user object stored in the session; and perhaps even whether a null gets stored instead of a user object when the user is not logged in, is none of the concern of this Session_Start function. All of that should be delegated out to an entirely different function, or set of functions.

 

In addition to authentication related code within global.asax and login.asax (which presumably just presents and processes the login form), you've also got some related code somewhere that runs on every page lode do you not? In your Session_Start code you're making a call to account.Login() which has a comment beside it mentioning that this returns false if a user has been banned, thus blocking login persistence from resuming their login. But unless you're doing a check on every page load, when you ban a user, that only prevents them from logging in again when they themselves log out, or resuming things when they let their session expire. While their session is still active, banning them has no effect. That is unless you're checking on every page load, or at least caching the result in the session and re-checking periodically (which would require an expiration check on every page load).

 

Furthermore, what if you implement a permissions system and you want to change the permissions of a user. Unless you fetch their permissions on every page load, the user's new permissions, whether you've enhanced them or reduced them, will only take effect if the user's session expires, or if they deliberately log out and back in again. Similarly other details cached in the user object, like username, may also present similar issues (if they can change their username or email, the old one being presented on pages after they changed it to a new one).

 

So if you're not fetching info about the user on every page load, perhaps you should revisit that. Assuming you have or add this additional code, where does this live?

 

I'd say that ideally, you probably want to extract this code from where it currently exists, group it all together in one place, and use the separation of concerns approach towards rewriting it, certainly removing any duplication. This may be a collection of functions within a single file, it may be a collection of methods with in a class, it may be spread across multiple classes and functions within a single file, it may be that it's best to split the code across multiple files, all are fine, but you should keep that file or those files specific to the task at hand. It may be that you have one file dedicated to all code for performing authentication, and a separate class in a separate file for user objects, which may hold properties and methods that don't have anything to do with authentication, hence why you've got it in a separate class and in a separate file. Then, the points in your program previously mentioned, the Session_Start event handler, the login form processing code, the block of code / function that executes on every page load to check ban/locked-out status and load permissions or whatever, as discussed above; each of these should utilise these authentication functions/classes to achieve their objectives, with the details of how it's done and the task of actually doing it being delegated and abstracted away.

 

I have a BasePage set up to where it gets the account object from the session, and checks security.  If something is wrong, it will redirect them to a page and display a message.

 

So should every entry point (Session_Start, button click events, Page Load/Init events, ...) just call methods and handle the errors?  I mean I might never add additional code to the Session_Start, so all it might do is check the persistent login.  It is still only doing one thing:  check a persistent login.  But I should wrap that in a method instead?

 

I guess that is why I always get confused about the "methods should only do one thing" argument.  What are the rules behind it?  Obviously a method can do more than just this:

int x = 3;

But take my code for example.  It is doing one task:  performing persistent login authentication.  Is it really worth it to wrap that in a method if I never will extend the Session_Start?  Should I wrap it in a method anyway in case I ever do want to?

 

The important logic is already grouped together in the Account class (which is why I call Account.Login() and the Account constructor).

[+theblazingangel MVC]

I have a BasePage set up to where it gets the account object from the session, and checks security.  If something is wrong, it will redirect them to a page and display a message.

 

So should every entry point (Session_Start, button click events, Page Load/Init events, ...) just call methods and handle the errors?  I mean I might never add additional code to the Session_Start, so all it might do is check the persistent login.  It is still only doing one thing:  check a persistent login.  But I should wrap that in a method instead?

 

I guess that is why I always get confused about the "methods should only do one thing" argument.  What are the rules behind it?  Obviously a method can do more than just this:

int x = 3;

But take my code for example.  It is doing one task:  performing persistent login authentication.  Is it really worth it to wrap that in a method if I never will extend the Session_Start?  Should I wrap it in a method anyway in case I ever do want to?

 

The important logic is already grouped together in the Account class (which is why I call Account.Login() and the Account constructor).

 

Sorry, yes, I meant to mention that you do already seem to be leaning towards the very model I'm describing, with your use of account and authentication classes, but I didn't know how much you are because I haven't seen any of the other code.

 

Should every entry point just be calling methods and handling errors? Well, I'm reluctant to try to push for that to be an absolute rule because there are often good reasons to break rules that may or may not be immediately clear, but here I think it would be a good design decision to move as much of the authentication routines and such all together in one location, and call that code from these entry points. My reasons for this should become clear below.

 

Regarding your confusion; Perhaps instead of thinking that a function should only "do one thing", it would help to think that a function should "have only one overall job / task / responsibility / purpose / objective". Now with an "entry point" type of function, the objective of the function may be to initialise things, to get stuff up and running. This may be very much generalised in the case of a program's main() function, or perhaps somewhat more specialised, for example in the case of your Session_Start function, the objective of which is specifically about setting up a new session. Such a function doesn't have to delegate every subtask it performs to a separate function, the task could be completed entirely within the entry point function; Whether you delegate / break-apart something or not comes down to other principles, including not allowing functions to grow too large or complex, re-usability, maintainability and separation of concerns.

 

Similarly, the properties and methods encapsulated within one class, and the collection of code within one file should all ideally be towards one purpose / responsibility / concern / whatever.

 

Consider the refactored solutions for your Session_Start function given in this thread. All of these solutions use return statements in an early-exit/multi-exit-point type of design. This is something which can potentially offer a means of writing a better piece of code, as demonstrated, but it has the constraint that it requires that the block of code that the return statement is a part of, is the only block of code within the function, or otherwise it inflicts complexity and may not even be compatible with the other code in the function, preventing its use.

 

Ideally, code should be separated such that it is possible to be able to refactor the code written to complete one task, with minimal to no impact on code written for other tasks and ideally without even needing to know anything about the details of other tasks. This is what the separation of concerns principle is about - breaking your code up into logical component sections so that they can be developed independently. This improves maintainability, readability, and re-usability.

 

Your Session_Start function is an entry point that although currently performs only one subtask, may at any point in the future need to support multiple sub-tasks. By complying with the principle of separation of concerns, it should be possible to add an additional sub-task to Session_Start without having to touch any code for the existing login persistence sub-task. If you were to assume that this existing sub-task will always be the only sub-task, and as such implement a refactored multi-exit-point design entirely within the function (such as the one I posted before realising the true purpose of Session_Start), and then later change your mind and wish to add a new sub-task, you may find that you need to refactor the login persistence sub task, or you have to move it to a separate function (even though that's simple to do, I wouldn't consider it to be in conformance with separation of concerns). Alternatively if you happen to be able to add the new sub-task without modifying the existing sub-task, you might be ending up with something too large and complex to remain as one function anyway, so again having to do something to the existing sub-task, against the separation of concerns principle.

 

What about in terms of re-usability? This particular block of code is only being used once within this particular application, there's no duplication, so we're okay from that point of view. But let's say that you or another programmer in your company (should it be company owned code) wished to reuse all of the authentication related code from this application in another application. And it may not necessarily be in a new application, but an existing one that is being refactored. Ideally it is best if as much of the code as possible can be incorporated into this new project by just copying across a set of files, and you can then integrate it into the new application by adding includes and some statements that make use of this library of authentication related routines and objects. The code should hopefully be designed in such a way as to reduce to as little as possible the amount of hacking things about that's necessary to get it to work in the new application. One thing that you may be forced to tweak is the SQL statements to conform to the design of a different database (it's probably not worth trying to build in enough abstraction to completely avoid it).

 

So specifically, how re-usable is your 'login persistence checking code for use upon new session creation'? Not very. You can't expect the global.asax file to be copied and pasted over the one in the other project. Your routine needs to be copied from this Session_Start function and pasted somewhere appropriate in the new one, either directly inside of it if possible, or in a separate function. The programmer doing this may be someone other than yourself, or it may be you, but who knows how much you'll remember of how you wrote all of this when you come back to it in the future. Who ever is going to use your code in this new project is going to have to know to do this, and is being unnecessarily exposed to the details of this routine in doing so (they need to understand that its use of return statements, presuming it uses them, may require that it be put into a separate function). All of this hurts re-usability. Let's take a moment to review what this routine does:

• Checks for the existence of a cookie
• Retrieves some attributes from a cookie
• Performs various bits of validation and error checking and handling
• Runs a security check of the user ID and token being valid together
• Creates a new account object
• Checks that the user isn't banned (and whatever else the account.Login() method does)
• Stores the account object in the session
• Redirects the user to the login page where necessary

That's a decent set of actions, a pretty useful routine, and an absolutely perfect candidate for being part of an authentication class/library. Assuming this were so; the name of the cookie used may be application specific, so application specific code could pass that to the authentication code somehow. Any "entry point" code needing to perform a "check login persistence" sub-task could simply call a method in the authentication class, and handle failure by doing the redirect. Routines for creation and removal of the cookie could also be part of the library. This design is much better in terms of re-usability as per what I described above, as well as being better in terms of separation of concerns.

[+Majesticmerc MVC]

There's a few questions here, so I'll try to answer them in order. Bear in mind though that a lot of this stuff is open to interpretation, so make your own decisions as to how far to take this advice :).

 

So should every entry point (Session_Start, button click events, Page Load/Init events, ...) just call methods and handle the errors?  I mean I might never add additional code to the Session_Start, so all it might do is check the persistent login.  It is still only doing one thing:  check a persistent login.  But I should wrap that in a method instead?

Personally, I think yes, an event handler, unless the code is trivially simple, should simply call other methods. For example, take a (simplified) session_start method:

 

protected void Session_Start(object sender, EventArgs e)
{
    PreloadUserData();
    UpdateSiteVisitStats();
}

In the example above, the Session_Start method performs two separate tasks:

• Preload user data (e.g. get data from the database, load existing login information, etc).
• Update site visit statistics (e.g. set in the database the user's ip address for audit purposes).

I can see this easily by reading the code! The code from those two methods may only be used in Session_Start, so the temptation is there to just code both things inside the Session_Start method. That would be a bad idea because it makes it harder to identify which part of Session_Start handles the preloading, and which part handles the updating of statistics. At a later date, it may also be necessary to add a third function to that event handler, it's much easier to simply add a third line of code to the event handler than it is to add another ten lines, and readability of maintained, which is always a good thing.

(The above example is perhaps not perfect because you can assign multiple methods as event handlers, however in the context of the original question I think it works)

I guess that is why I always get confused about the "methods should only do one thing" argument.  What are the rules behind it?  Obviously a method can do more than just this:

int x = 3;

Of course, a method can always be more than one line, but you have to consider what you mean by "one thing". When we say that a method should only do one thing, what we actually mean is that a method should only perform one task. So a method has a job to do, and it should only do that job. To use a car analogy, you wouldn't expect the accelerator to turn the steering wheel!

To extend the car analogy, consider an object called "Car" that has a method called "Accelerate". The Accelerate method has a single job, to make the car increase it's rate of acceleration. The code for the "Accelerate" method might be a million lines long (protip: don't ever write a method that is a million lines long), but it should only perform the task of Acceleration.

A rule I once heard was this:

"Every time you use the word 'and' when describing your function to someone, it should be split into two functions."

To use this in your specific example:

It is doing one task:  performing persistent login authentication.  Is it really worth it to wrap that in a method if I never will extend the Session_Start?  Should I wrap it in a method anyway in case I ever do want to?

While you could argue that you're only doing "one thing", I'd disagree. You're doing several things:

• Getting the authentication Cookie
• Fetching user credentials from the cookie.
• Validating credentials
• Checking that user account is valid.

Notice that the four things that were listed above are all direct causes of the repetition. If the authentication cookie isn't available, ResetAuthentication() is called. If credentials are not available (or not valid), ResetAuthentication() is called, and finally, if the user account is not valid, ResetAuthentication is called.

As far as I can see, there are four separate tasks going on here.

The important logic is already grouped together in the Account class (which is why I call Account.Login() and the Account constructor).

You're quite correct, it is all grouped together, however I'd argue that you've grouped too much stuff together.

I hope this helps :)

BTW this has been a great thread to read. Nice work :)

[simplezz]

Eloquently put theblazingangel. I generally employ a similar strategy. Sometimes it can require some juggling around and rewriting to perfect it (time permitting). The thing to remember is, you don't have to get it right the first time. It might take a few tries. You do get better at it overtime though if you get in the habit of doing it. Thinking about the structure of it and assigning responsibilities and roles from the beginning really helps in that regard.

 

On the topic of out parameters, it's a good practise in my opinion. It makes the code self-documenting / self-explanatory because you return a simple result indicating success or failure (boolean or custom result) from which the caller can make the decision to exit immediately negating the requirement for complex nested statments, and you can receive objects / data structures as well. I do it all the time in C. The only difference is, in C, you pass a pointer to a pointer (**) if the callee allocates the memory. All pretty standard stuff.

/* https://en.wikipedia.org/wiki/Extended_Boot_Record#Values
 * an EBR's location is the relative offset of an extended entry's first sector within the extended partition.  
 * a logical partition's location is the relative offset between its EBR and its lba.
 * */
PRIVATE FRESULT
partition_read_extended ( BlockDevice *dev, Partition *in, int fd, PartitionTable **out )
{
    _EBR            ebr;
    PartitionTable  *ext;
    uint64_t        ebr_lba;
    int             ext_id = EXT_START_ID;
    
    ext = malloc (   sizeof ( PartitionTable ) );
    memset ( ext, 0, sizeof ( PartitionTable ) );
    
    ext->dev = dev;
    ebr_lba  = in->sectors.first;
    
    do {

        lseek ( fd, ebr_lba * SECTOR_SIZE, SEEK_SET );
        
        if ( FFAILURE == partition_read_mbr ( dev, fd, &ebr ) )
            return FFAILURE;
        
        partition_read_record ( ext, &ebr.entries [ 0 ], ebr_lba, ext_id++ );  
        
        ebr_lba = ebr.entries [ 1 ].lba + in->sectors.first;  
        
    } while ( EXTENDED == ebr.entries [ 1 ].type );

    *out = ext;

    return FSUCCESS;
}

[xWhiplash]

There's a few questions here, so I'll try to answer them in order. Bear in mind though that a lot of this stuff is open to interpretation, so make your own decisions as to how far to take this advice :).

 

Personally, I think yes, an event handler, unless the code is trivially simple, should simply call other methods. For example, take a (simplified) session_start method:

 

protected void Session_Start(object sender, EventArgs e)
{
    PreloadUserData();
    UpdateSiteVisitStats();
}

In the example above, the Session_Start method performs two separate tasks:

• Preload user data (e.g. get data from the database, load existing login information, etc).
• Update site visit statistics (e.g. set in the database the user's ip address for audit purposes).

I can see this easily by reading the code! The code from those two methods may only be used in Session_Start, so the temptation is there to just code both things inside the Session_Start method. That would be a bad idea because it makes it harder to identify which part of Session_Start handles the preloading, and which part handles the updating of statistics. At a later date, it may also be necessary to add a third function to that event handler, it's much easier to simply add a third line of code to the event handler than it is to add another ten lines, and readability of maintained, which is always a good thing.

(The above example is perhaps not perfect because you can assign multiple methods as event handlers, however in the context of the original question I think it works)

Of course, a method can always be more than one line, but you have to consider what you mean by "one thing". When we say that a method should only do one thing, what we actually mean is that a method should only perform one task. So a method has a job to do, and it should only do that job. To use a car analogy, you wouldn't expect the accelerator to turn the steering wheel!

To extend the car analogy, consider an object called "Car" that has a method called "Accelerate". The Accelerate method has a single job, to make the car increase it's rate of acceleration. The code for the "Accelerate" method might be a million lines long (protip: don't ever write a method that is a million lines long), but it should only perform the task of Acceleration.

A rule I once heard was this:

"Every time you use the word 'and' when describing your function to someone, it should be split into two functions."

To use this in your specific example:

While you could argue that you're only doing "one thing", I'd disagree. You're doing several things:

• Getting the authentication Cookie
• Fetching user credentials from the cookie.
• Validating credentials
• Checking that user account is valid.

Notice that the four things that were listed above are all direct causes of the repetition. If the authentication cookie isn't available, ResetAuthentication() is called. If credentials are not available (or not valid), ResetAuthentication() is called, and finally, if the user account is not valid, ResetAuthentication is called.

As far as I can see, there are four separate tasks going on here.

You're quite correct, it is all grouped together, however I'd argue that you've grouped too much stuff together.

I hope this helps :)

BTW this has been a great thread to read. Nice work :)

 

 

Thanks.  When you say I have grouped too much, is that just too much for a single method to do?

 

And thanks to everyone else.  I try to learn and increase my skills in areas I have trouble with, and code repetition (and I guess method performing a single task) is one of my problems. 

[astropheed Veteran]

This thread has actually helped raise my confidence in the way in which I handle things. Excellent thread, great question. (y)

 

Confidence+1

[simplezz]

I try to learn and increase my skills in areas I have trouble with, and code repetition (and I guess method performing a single task) is one of my problems. 

You're not the only one :) If there's one thing I've learnt in this life, it's that I know nothing. I'm constantly learning and correcting myself. Don't be afraid to ask questions or make mistakes, for we all do the latter :laugh:

[astropheed Veteran]

You're not the only one :) If there's one thing I've learnt in this life, it's that I know nothing. I'm constantly learning and correcting myself. Don't be afraid to ask questions or make mistakes, for we all do the latter :laugh:

 

The first thing I learn when I learn anything is how much I didn't know. The second thing I learn is how wrong I was when I thought I learned it. Then I start to learn it.