Open Side Menu Go to the Top
Register
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** ** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD **

08-17-2011 , 12:28 AM
Quote:
Originally Posted by gaming_mouse
mac users, is it still true that there is no way to maximize a window? and is apple still trying to sell this as a "feature"? or did they finally give in and implement maximize?
Lion lets you fullscreen an app...if that's what you mean.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 07:46 AM
Quote:
Originally Posted by gaming_mouse
It's only a problem if the helper functions have problems in them.
I feel like this is taking on the form of a religious debate. You're honestly telling me that if you're going to be modifying or extending code in a class you don't take the time to look at all of the functions in that class? That doesn't pass my common sense test.

Quote:
Originally Posted by gaming_mouse
And anyway if you do the way I suggest, even if you are interested in modifying the implementation code of helper functions for whatever reason, you will find the code faster.
Again, this is your claim. I don't understand at all how you can claim that pressing F3 to take you to your code is faster than the code already being where your cursor is. Maybe it's true for you, but its far from an absolute truth in general.

Quote:
Originally Posted by gaming_mouse
Like I said though, this is silly without a concrete example.
I'll see if I see any today that I can grab and post.

Quote:
Originally Posted by gaming_mouse
Why don't you paste in some function that does multiple things, that you think is better as one big function. I will try to convince that it's better broken down with a specific refactoring.
I'm not advocating for big functions. I'm not saying it's better than a well thought out refactoring. I'm claiming that there are lots of bad refactorings out there and that just because they use small functions doesn't mean they're better than the equivalent big function.

Edit: Bolding the important part that I feel isn't clear between us.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 07:55 AM
Some bigger continuous pages are preferable in my opinion sometimes (but quite specific/rare). I just wrote an IPN receiver for my website, it's contained, specific, and very easy to debug line by line. I don't mind it being unmodular in this instance but generally of/c favour modular design.

There definitely are people who over modularise things though which ends up obfuscating everything needlessly but it is quite rare to come across from my experience.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 08:11 AM
Quote:
Originally Posted by Neil S
PHP's haphazard, irregular, randomly constructed library is one of its demerits.
What's great about PHP is that other PHP developers won't chastise you for writing 1000 line functions. It's like a license to ignore OOP. lolwebdeveloperments
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 09:02 AM
Quote:
Originally Posted by jjshabado

I'm not advocating for big functions. I'm not saying it's better than a well thought out refactoring. I'm claiming that there are lots of bad refactorings out there and that just because they use small functions doesn't mean they're better than the equivalent big function.

Edit: Bolding the important part that I feel isn't clear between us.
jj,

as i said the debate is silly at this point without examples. but since you took the time i'll respond to this. you seem to be worried about the programmers who have taken the time to learn about refactoring, but not taken the time to *really* learn, and hence misapply it in their over-zealousness to modularize everything.

i'm not saying this can't be bad, it's just not what i'm generally worried about. i rarely come across it, and when i do it usually has some sense to it, because the authors actually care about the code on some level and are trying to do good even if they suck at it.

the far far more common error is people who have learned to code but never made any effort to learn about standards or refactoring or best practices. they just make all the classic errors that OO patterns and best practices were designed to avoid. duplicate code, lack of encapsulation, tight coupling of unrelated things.

this is the bread and butter of bad code, and as a programmer these are the people whose code will cause you the most pain and whose code you will encounter far more often.

i guess ymmv. but that's my experience.

EDIT: so yeah, i agree with the bolded part in theory. but if you told my one coder wrote average 50 line functions one coder wrote avg 5 line functions, and i had to choose which coder's project to work on without any more info, i'd choose the 5 line guy in a heartbeat.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 09:15 AM
Here's a good example from earlier in the thread. We have the code written in 3 ways. The worst of these (to me) is the one that was caused by over zealous refactoring.

It's something that I come across quite a lot in Java (Overzealous following of rules/patterns without thought into why the rule/pattern is being applied).

Like you said though: ymmv.


No refactoring:
Code:
def blowUp(x, y, z):
    if x < y and x < z:
        a,b = y, z
    elif y < z and y < x:
        a,b = z, x
    else:
        a,b = x, y
    print (a * a +  b * b)
Overzealous refactoring:
Code:
def blowUp(x, y, z):
    a, b = andCompare(x, y, z)
    sol = addSquare(a, b)
    print sol

def compare(a, b):
    if a < b:
        return True
    return False

def andCompare(a, b, c):
    if compare(a, b) and compare(a, c):
        return b, c
    elif compare(b, c) and compare(b, a):
        return c, a
    return a, b
    
def square(x):
    return x*x

def addSquare(x, y):
    return square(x) + square(y)
Hybrid:
Code:
def blowUp(x, y, z):
    a,b = dropLowest(x,y,z)
    print (a * a +  b * b)

def dropLowest(a,b,c):
  if a < b and a < c:
    return b,c
  elif b < a and b < c:
    return a,c
  else:
    return a,b
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 09:17 AM
Quote:
Originally Posted by gaming_mouse
EDIT: so yeah, i agree with the bolded part in theory. but if you told my one coder wrote average 50 line functions one coder wrote avg 5 line functions, and i had to choose which coder's project to work on without any more info, i'd choose the 5 line guy in a heartbeat.
Pure curiosity, around where do you switch?

If I'm told someone wrote average 4 line functions and someone wrote average 25 line functions - I'm probably going with the 25 line dude.

Edit: Or dudette. Not sure if we've had a female post in this forum...
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 09:34 AM
Quote:
Originally Posted by jjshabado
Pure curiosity, around where do you switch?

If I'm told someone wrote average 4 line functions and someone wrote average 25 line functions - I'm probably going with the 25 line dude.

Edit: Or dudette. Not sure if we've had a female post in this forum...
yeah i dunno, i probably still go 4 but its closer, irl i'd want to see the actual code and make an informed decision, my hypothetical was just a way of conveying the central point i wanted to make.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 09:37 AM
It's just silly imo to say 4 line functions > n line functions. In ASP.net this is a totally standard method:
Code:
    /// <summary>
    /// Given a list of category ID's return all specs belonging in them
    /// </summary>
    public static Lite.Spec[] getSpecsForCats(int[] CategoryIDs)
    {
        Lite.Spec[] Specs;

        using(CrystalCommon.MainContext db = new CrystalCommon.MainContext()){
            
            // Get distinct list of all specifications
            var q = (from c in db.tblLiteCategorySpecs
                     join s in db.tblSpecifications on c.SpecID equals s.id
                     where CategoryIDs.Contains(c.CategoryID)
                     select new
                        { s, c })
                    .ToList();
            Specs = new Lite.Spec[q.Count()];
            int i = 0;
            foreach (var Rec in q)
            {
                Specs[i] = new Lite.Spec(Rec.s);
                Specs[i].FriendlyName = Rec.c.FriendlyName;
                Specs[i].CategoryID = Rec.c.CategoryID;
                i++;
            }
        }
       
        return Specs;
    }
I don't see at all how anyone could say it's bad because it's over 4 lines, it's concise and functional, I mean I could probably put it in 4 lines if I were to over modularise it but it would be a massive waste of time and an unmaintainable mess!

Number of lines per method wouldn't factor in at all if I was in recruitment. My favourite Bill Gates quote:

Quote:
Measuring programming progress by lines of code is like measuring aircraft building progress by weight
I think it applies to this debate!
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 09:42 AM
Not entirely. I mean, I could easily see building the query in a separate function which I think is a pretty big win in terms of readability of that code.

That's probably where I'd stop though (although reasonable people could make an argument for a second 'helper' function that processes the results - making your original function very trivial).
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 09:48 AM
Putting the query in a separate function (assuming we're not talking about stored procedures here) and a helper function to process results would be what I consider unnecessary over modularisation, I think it would be the equivalent of a 1-1 database relationship (99% of the time a 1-1 relationship should just exist in the same table).

Quote:
I mean, I could easily see building the query in a separate function
The way I look at my query, is that it is in a separate function!
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 09:59 AM
Ok... I don't know ASP.net at all. So I might be doing something ******ed. But I lean towards this level of refactoring.

It seems a bit clearer to me and allows me to easy unit test the non-db part.

I'd also probably (again not sure exactly how things work in ASP.net) move the query to a constant of some sort so that I could easily look over my existing queries in one place.

Code:
    /// <summary>
    /// Given a list of category ID's return all specs belonging in them
    /// </summary>
    public static Lite.Spec[] getSpecsForCats(int[] CategoryIDs)
    {
        Lite.Spec[] Specs;

        using(CrystalCommon.MainContext db = new CrystalCommon.MainContext()){
            
            // Get distinct list of all specifications
            var q = (from c in db.tblLiteCategorySpecs
                     join s in db.tblSpecifications on c.SpecID equals s.id
                     where CategoryIDs.Contains(c.CategoryID)
                     select new
                        { s, c })
                    .ToList();
            Specs = new Lite.Spec[q.Count()];
            int i = 0;
            foreach (var Rec in q)
            {
                Specs[i] = new Lite.Spec(Rec.s);
                Specs[i].FriendlyName = Rec.c.FriendlyName;
                Specs[i].CategoryID = Rec.c.CategoryID;
                i++;
            }
        }
       
        return Specs;
    }
Code:
    /// <summary>
    /// Given a list of category ID's return all specs belonging in them
    /// </summary>
    public static Lite.Spec[] getSpecsForCats(int[] CategoryIDs)
    {
            var q = getDbSpecs(categoryIDs);
            return buildSpecsFromDbQuery(q);
    }

    private static List getDbSpecs(int[] CategoryIDs) {
         using(CrystalCommon.MainContext db = new CrystalCommon.MainContext()) {
            
            // Get distinct list of all specifications
            return (from c in db.tblLiteCategorySpecs
                     join s in db.tblSpecifications on c.SpecID equals s.id
                     where CategoryIDs.Contains(c.CategoryID)
                     select new
                        { s, c })
                    .ToList();
         }
      }

      private static List.Spec[] buildSpecsFromDbQuery(var q) {
            Specs = new Lite.Spec[q.Count()];
            int i = 0;
            foreach (var Rec in q)
            {
                Specs[i] = new Lite.Spec(Rec.s);
                Specs[i].FriendlyName = Rec.c.FriendlyName;
                Specs[i].CategoryID = Rec.c.CategoryID;
                i++;
            }
            return Specs
    }
Edit: Crazy* people would then try to separate out constructing a single record, and break that up into individual functions for setting properties.



* Crazy obviously means someone that doesn't 100% agree with me.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:04 AM
Quote:
Originally Posted by Gullanian
Putting the query in a separate function (assuming we're not talking about stored procedures here) and a helper function to process results would be what I consider unnecessary over modularisation, I think it would be the equivalent of a 1-1 database relationship (99% of the time a 1-1 relationship should just exist in the same table).
But there are lots of reasons to break a function up. I've given two above:
1) I want to test functionality separate from actual database logic
2) I want to have my DB queries in one place (or at least by themselves) so I (or a DBA) can look over them easily.

My problem is when people can't give me a reason for breaking up a function besides "It makes it smaller".
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:04 AM
yes obv i agree that example is bad. EDIT: meaning the lisp example from dave, i still have to read the other ones
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:07 AM
I get what you are saying, maybe this is just difference of opinion (but I strongly suspect not), but because I consider those functions to have a one-to-one relationship with each it is totally unnecessary. You can often pre-empt segments of functions to modularise, but this doesn't mean you need to pre-empt everything, a lot of functions like the one I provided function just great on their own and can be modularised when the time comes to do so, not before.

You say it would benefit readability, but I honestly think that the original is a lot more readable. Which makes me think perhaps this is a style/preference issue but I don't think it is if I'm going to be honest about it!
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:08 AM
Quote:
Originally Posted by gaming_mouse
yes obv i agree that example is bad. EDIT: meaning the lisp example from dave, i still have to read the other ones
But that was literally the level of refactoring I was encountering when I went on my rant!

To Dave: See, your little learning exercise ended up with code just as good as what a lot of 'professional' Open Source contributors do. So you're not doing too badly.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:10 AM
Quote:
Originally Posted by Gullanian
You say it would benefit readability, but I honestly think that the original is a lot more readable. Which makes me think perhaps this is a style/preference issue but I don't think it is if I'm going to be honest about it!
I do think the refactored one is a bit more readable - but I totally accept that that is a personal preference. In the end I went with the two reasons I listed (which I think are valid) for why I'd want to refactor the function.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:13 AM
In timed tests to see how long it takes a group of programmers to tell me what each example does, the original I provided would win versus the 3 smaller functions by a significant margin imo. I think that's what readability is at the end of the day although definitions may vary.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:16 AM
Gullanian:

1) Do you agree or not with my two reasons for refactoring the function?
2) What answer are you looking for? I mean isn't the answer just:

Code:
/// <summary>
/// Given a list of category ID's return all specs belonging in them
/// </summary>
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:31 AM
Quote:
Originally Posted by jjshabado
Not entirely. I mean, I could easily see building the query in a separate function which I think is a pretty big win in terms of readability of that code.

That's probably where I'd stop though (although reasonable people could make an argument for a second 'helper' function that processes the results - making your original function very trivial).
This. jj, I think in practice we're gonna be on the exact same page about most things.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:34 AM
Quote:
Originally Posted by Gullanian

You say it would benefit readability, but I honestly think that the original is a lot more readable.
wrong.

Quote:
a style/preference issue but I don't think it is if I'm going to be honest about it!
absolutely correct!
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:34 AM
Quote:
Originally Posted by Gullanian
Some bigger continuous pages are preferable in my opinion sometimes (but quite specific/rare). I just wrote an IPN receiver for my website, it's contained, specific, and very easy to debug line by line. I don't mind it being unmodular in this instance but generally of/c favour modular design.
shabby has hit this but for me the punch line is: show me your unit tests, and i'll tell you how well your code is architected.

fwiw i generally agree with mouse's position: the magnitude of failure is much higher in code that is too big vs too small, the frequency of failure skews toward too big rather than too small, hence big code is "worse" than small code (has larger -EV).

shabby, i appreciate your side but dredging up an example that began as a hw problem rather than a real-world problem illustrates exactly how hard one has to try to produce code that is "too refactored". let's see some real world code, mang!
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:38 AM
The question I would ask would be "tell me how it works" I suppose.

For an application where you want to separate DB logic from code logic further I would favour a stored procedure over modularising every query in code. I suppose for your points I do agree in principal but I think it depends largely on the project.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:41 AM
Quote:
Originally Posted by gaming_mouse
wrong.
I mean, when we are talking about something subjective like readability, is it not perhaps a bit arrogant to declare my opinion is flat out wrong?

Perhaps you could start by defining readability, because I think being able to explain what a piece of code does in the fastest time possible is a pretty strong definition.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote
08-17-2011 , 10:49 AM
Quote:
Originally Posted by Gullanian
I mean, when we are talking about something subjective like readability, is it not perhaps a bit arrogant to declare my opinion is flat out wrong?

Perhaps you could start by defining readability, because I think being able to explain what a piece of code does in the fastest time possible is a pretty strong definition.
it is, i was being deliberately provocative, though I do think it is a pretty clear cut case of better and worse. and by the definition you just gave, jj's refactoring wins imo. The main reason is that it gives you immediate access to a high-level description of the code, which gets lost in implementation details in your version. So what does the code do?

Code:
var q = getDbSpecs(categoryIDs);
            return buildSpecsFromDbQuery(q);
"Oh, it grabs specs based on category ids from the db, then it builds those specs based on the results"

Now if you want to know more detail about one of those things, you can drill down. But the key point is exposing a high-level summary of the algorithm, and then allowing the reader of the code to easily get more detail about one specific piece of it if he wants. The high-level summary is what's missing from the original code.

EDIT: I would add that in your case the "error" is small. That is the original code is still readable, it only does 2 things, it's totally fine. But the refactored version is an improvement.
** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** Quote

      
m