Another dependency question

PHP
1

Been toying with this with my unit tests.


class Bar {
  protected $debug = DEBUG;

  public function toggleDebug( $debug ) {
    $this->debug = $debug ? true : false;
  }
}

This allows a global constant of the framework to seemlessly apply a debug mode to classes (or at least the ones that care about the debug state. Many do not and therefore do not have the method and member above).

But it’s a non-committal approach, the debug can be toggled after instantiation freely, which my unit tests do to make sure the object is behaves as expected in both modes with one test.

Also, I’ve only done this with boolean toggles.

2

Hi…

Actually I do have a big screen :). I tend to use a straight editor rather than an IDE, so it’s all used for text. My font size is big though. I have rubbish eyesight. Currently I can see 59 lines.

Not every class I write these days is one page I have to admit, but I definitely want to see the core without scrolling. Exceptions? The odd one line helper method will be below the fold on occasion. And methods with nothing but declarations I allow to get long. And wrappers/facades as can be just a big shopping list as all they do is delegate. Anything mechanical though, I definitely want small.

Because it’s probably not expressing the task very well. No one would write a paragraph that long (even in the most retarded specification?), so I couldn’t believe it’s mapping to the problem space correctly.

I’d definitely want to play with it at 350 lines. Can we take out the mechanics and reduce it to a prescription? Can we group a bunch of methods that are sharing data and shove them into another class? Are we doing a bunch of marshalling that is unnecessary?

I find a real qualatitive difference between a one page class and a many page one. You take the one pager in at a glance and carry that visual snapshot around as you look around the rest of the code. The size difference between obvious and confusing is so small.

I don’t feel comfortable guessing anymore. I know I’ll have to fix the code many times if I lose mastery over it.

yours, Marcus

3

:slight_smile:

4

Marcus just got out after a ten year stretch in troll prison and is trying to turn his life around with a new career as a programmer. He’s trying really hard so please give him a chance.

(sorry… :D)

5

wow. That’s a great reason to refactor. I suppose the other alternatives are getting a screen with a higher resolution or reducing your IDE’s font size :wink:

On a serious note, if all the code in a class is actually related to the task at hand why refactor it?

6

Oh dear. :shifty:

7

I use the address ‘sir’ and get called ‘squire’ in return. A very mild insult sure, but I have better things to do than explain myself to someone so condescending and elitist.

As TomB pointed out - if all the code is on target for the class who cares if it’s 90 lines or 900 lines?

So when someone is trying to push buttons and is being disrespectful I put them in the ignore list without hesitation because I do not like being baited. Besides, with the comments lastcraft has made I seriously doubt he has any programming experience beyond academic exercises and theory, but since he holds that experience as being more valuable than the real world of having built, sold and maintained a program before there’s a problem.

I’ve seen too many code bases written by school boys in love with their own intelligence and obfuscated beyond hope of being useful in my time, and I have little interest in advice from such people who lack practical experience. Is lastcraft one of those? I don’t know, but he sure posts like it.

Folks, it’s not enough to know the “how” of all the latest design patterns and them follow them blindly and religiously. You also need to know “why” so that you can recognize when a pattern is not applicable to the situation at hand.

A one of instance in one class 3 lines long including braces is not cause to write a 500 line debugging subsystem spread over 5 files. If it comes up again then is the time to consider its addition.

8

How you organize it is really up to you, but the whole point is to actually split the code up into components with specific roles. That was 7 lines BTW, and not a factory pattern. sigh

Personally, not a fan of the HTML comments for template names. It usually gets abused, and sadly, often IN production environments. I guess it depends how you structure your HTML, but I find it more annoying than useful. I’d rather test the output of a single component rather than try and dig through the entire response riddled with comments for it.

Trolled? Are you posting for feedback, or just to hear yourself speak?

9

Hi…

I was just thinking aloud.

But you aren’t giving me the benefit of the doubt :). As I said, in my case that’s 8% overhead.

I just wouldn’t do it. I’m bemused why you have to tell yourself your own template name.

It really isn’t. Show me your business requirements that say you want a debug statement that spits out the template name.

Er…no it isn’t squire. The user will be profoundly disinterested upon seeing this.

Well cheers, but I’m not the one ranting.

As I said, a 350 line class is long for me. Insanely long. Most classes fit on a screen and if spreads over two I refactor.

I didn’t mention patterns.

I’d do it in zero lines. I might add a unit test to prove I can find the same template I specified in the first place, but I wouldn’t place these switches in production code. I don’t like my apps changing behaviour across the board when I toggle a flag. Confuses the hell out of people.

yours, Marcus

10

Do you have any idea how petty that comes across? I’ll give you the benefit of the doubt since its late and I’m rather sore and stressed out to begin with, but jeez - 3 lines. My God it’s the end of the world.

How would you do it?

Coincidently, the function is declared equal to the constant DEBUG which is set by the framework long before this class gets instantiated.

There is the conceptual load too, of having behaviour not related directly to the task and of adding an extra object variable.

Not related to the task? Seriously? Labeling the template you are inserting into the output HTML during debug is very much related to the task of “composing an HTML response” sir. And there is a reason why the line is where it is.

Parse template gets called at least once to parse the page template. But it may not be called for each template. For example, a template might read


<table>
  <? foreach ($summary as $row):
    include($this->findTemplate('row');
  endforeach; ?>
</table>

Or if I need a new scope for those rows I would do this.


<table>
  <? foreach ($summary as $row):
    $this->parseTemplate('row', $row);
  endforeach; ?>
</table>

The former case is the most frequent, hence the reason find template inserts the comment - because that function is guaranteed to be called.

Sorry, i forgot to ask how many there were already.

My style is for classes to be shorter (2 pages top), so for me about 8%.

Hm…still dont like it. I’m a bit of a code quality purist though. What are you using this for?

yours, Marcus

There’s being a purist, and then there’s being insane. A lot of frameworks suffer from pattern overkill, where the pattern is followed so religiously it bloats the code every bit as badly as if it wasn’t properly factored to begin with.

Go ahead and prove you can do it in less than 4 lines, but I’m betting you’ll put across some factory pattern like Adrian did that trades 4 lines in this class for 40 lines elsewhere that have to be dealt with whether or not DEBUG is on.

11

Hi…

I try to avoid this, but this might be an irrational fear. Doesn’t this mean your classes are doing two things? The original job plus printing a bunch of printing stuff?

Isn’t this a bit cluttered? I’m not being facetious here, I’m genuinely curious. How big is the class, and how many lines of it are debug related?

yours, Marcus

12

Hi…

That’s 3 lines with the “if”. Plus I assume you declare the thing to be false at the top. So that’s 4 out of 200-ish allowing for waste of a typical PHP brace style, or about 2%.

There is the conceptual load too, of having behaviour not related directly to the task and of adding an extra object variable. Sorry, i forgot to ask how many there were already.

My style is for classes to be shorter (2 pages top), so for me about 8%.

Hm…still dont like it. I’m a bit of a code quality purist though. What are you using this for?

yours, Marcus

13

Do you mean this?

print($responder->findTemplate($template));

findTemplate resolves a template’s name to it’s path. Whenever it is called it is wrapped with include(). There is a whole other function to just see if a template exists.

It can’t do the including directly because of scope. If the include statement is inside findTemplate then the included template can only see the variables inside of findTemplate which is more or less useless. So it must instead be wrapped by the include statement. Meanwhile if a new scope is wanted parseTemplate can be called. In that function you can clearly see how findTemplate is used.

14

The flag bugs me too.

Off Topic:

Another thing that bugs me is that findTemplate() function, it should do the including directly if the print statement is supposed to prefix. Otherwise findTemplate() has what I call a side effect. (Always outputting the html comment assuming the template is going to be included next.)


 protected function includeTemplate($template ) {
        $path = $this->findTemplatePath( $template, true );
    
        // Die if still null
        if (is_null($path)) {
            throw new MissingTemplateException( $template );            
        } else {            
            // Place an html comment for all templates but the master template.
            if ( $this->debug && $template != $this->masterTemplate ) {
                print("<!-- Begin Template: $template -->");
            }
      
            if (!include($path))
                throw new FatalException('Unable to read template file '.$template);   
        }
    }

...

   protected function parseTemplate($template, array $output = null) {
        
        // Extract the global output first.
        extract($this->output);
        
        // Now the template's local output, which overwrites any global stuff.
        if (is_array($output)) {
            extract($output);
        }

        ob_start();
        try {
            $this->includeTemplate($template);
            return ob_get_clean();
        }
        catch (Exception $exception) {
...
        }
    }
15

Could you start over at the beginning and define why you need a debug mode for your classes? What’s the purpose? Wouldn’t you be better off isolating the functionality that you need to “debug” in your unit tests?

16

Well yeah, but I don’t typically bother with semantics on a vbulletin post.

Depending on your application flow, your objects may be instantiated within an object which scopes the application, or maybe a registry object (Not singleton), so then you could have something like this:

class Application{
     protected $Debugger = null;
     public function __Construct(iDebugHandler $Debugger = null){
         if(is_null($Debugger)){
             $Debugger = new NullDebugHandler();
         }
         $this->Debugger = $Debugger;
     }
     function something(){
         $YourClass = new YourClass($this->Debugger);
     }
}

Thoughts?

Overcomplicated nor solves the problem at hand. The class will still end up making debug calls, worse you’ve polluted the production environment with the need to pass a useless null object around for a function only used in debug.

In my setup many classes aren’t use debug. One of the few that does is the HTMLResponder in the findTemplate method.


protected function findTemplate($template) {
  // Lots of code not related to example here.
  if ($this->debug) {
    echo "<!-- Begin Template: $template -->" before returning a template. 
  }
}

If someone uses the class outside the framework and forgets to set DEBUG then they aren’t going to see debug messages - the production default behavior. It isn’t the end of the world.

On second thought, the best approach is perhaps no handling code at all. The reason being I set up unit tests with an testing extension.


class Responder {
  protected $debug = DEBUG;
}

class ResponderUnitTest extends Responder {
  public function test() {
    $this->debug = false;
// test with debug off.
    $this->debug = true;
// test with it on..
  }
}

So toggling isn’t an issue. (I should note here I run tests both inside and outside class scope using this approach).

The DEBUG flag becomes hidden away during normal operation. How bad is this? I’m thinking for this case it isn’t. I certainly wouldn’t want a vital parameter to be handled this way though - I’d want it to be fairly visible.

17

Responder handles the HTTP 1.0 protocol.
HTMLResponder parses templates in preparation for an html response.

Here’s the code of findTemplate and the related class parseTemplate


	/**
	 * Locate a template file and return its path.
	 * If the system is in debug mode this function echo's an html comment
	 * to label the template start. For this reason do not call this function
	 * outside of an output buffer.
	 *
	 * @param string $template
	 * @return string
	 */
	protected function findTemplate( $template ) {
		$path = $this->findTemplatePath( $template, true );
	
		// Die if still null
		if (is_null($path)) {
			throw new MissingTemplateException( $template );			
		} else {			
			// Place an html comment for all templates but the master template.
			if ( $this->debug && $template != $this->masterTemplate ) {
				print("<!-- Begin Template: $template -->");
			}
			
			return $path;
		}
	}
			
	/**
	 * Parse a template. 
	 * 
	 * Usually when templates need to nest other templates
	 * they will call #findTemplate directly and include it, but this function
	 * can be used instead to create a new scope.  $this in any template
	 * refers to the responder. Also output arrays should not have numeric 
	 * keys or the keys 'this', 'template' or 'output'.  If you
	 * do this and it will error. Rename you keys - leave this function alone.
	 *
	 * @param string Template path
	 * @param array output variables.
	 */
	protected function parseTemplate($template, array $output = null) {
		
		// Extract the global output first.
		extract($this->output);
		
		// Now the template's local output, which overwrites any global stuff.
		if (is_array($output)) {
			extract($output);
		}

		ob_start();
		
		if (!include($this->findTemplate($template))) {
			throw new FatalException('Unable to read template file '.$template);	
		}
		
		return ob_get_clean();
	}

The whole class is only 350 lines including comment text, maybe 250 without the commentary. You can see the only line in the HTMLResponder that cares about the debug state.

18

It’s pretty obvious you are not. You’ve got a fantastic opportunity here if only you realised it.

19

To be really picky, technically the toggle shouldn’t accept a parameter because a toggle on a boolean simply sets the existing value as the NOT of the current value:

public function toggleDebug() {
    $this->debug = !$this->debug;
}
public function setDebug($debug = DEBUG){
    $this->debug = (bool)$debug;
}

Though personally I wouldn’t use this approach because this allows the class to handle the debugging functionality, which I don’t like.

Rather, I would have an iDebugHandler interface. Pass this to a $DebugHandler variable. The default is a NullDebugHandler object which does zip, zadda, zilch (nothing).

interface iDebugHandler{
    function output($Message, $Object = null);
}
class StandardDebugHandler implements iDebugHandler{
    public function output($Message, $Object = null){
        echo '<h3>Debug Output</h3>';
        echo '<p>', $Message, '</p>';
        if(!is_null($Object)){
            echo '<h4>Sender:</h4>';
            echo '<pre>';
            var_dump($Object);
            echo '</pre>';
        }
    }
}
class NullDebugHandler implements iDebugHandler{
    public function output($Message, $Object = null){}
}

Depending on your application flow, your objects may be instantiated within an object which scopes the application, or maybe a registry object (Not singleton), so then you could have something like this:

class Application{
     protected $Debugger = null;
     public function __Construct(iDebugHandler $Debugger = null){
         if(is_null($Debugger)){
             $Debugger = new NullDebugHandler();
         }
         $this->Debugger = $Debugger;
     }
     function something(){
         $YourClass = new YourClass($this->Debugger);
     }
}

Thoughts?

20

While that uses polymorphism, it breaks up in the face of extensive polymorphism. HTMLResponder extends Responder wouldn’t see that Debug extension. Extending the child classes isn’t feasable since there’s no way of knowing what those are going to end up being, and in any event you’d have to write a factory class to bind on the debugger. This in turn leads to overwrought boilerplate nonsense code and overcomplexity.

What you have in mind works fine in JavaScript where you can modify an object’s prototype thanks to the prototypical inheritance model, but not so well in PHP.

But finally, if a debug system needs a lot of special behaviors it isn’t going to be very effective because it will fail to accurately resemble the production environment. Hence the times when the debug flag gets referenced should be few and far between. HTMLResponder and the Database class are the only two that care (The database class times and logs queries when debugging) in my system, and most of it doesn’t care nor should it care.