Remove the 3000-lines long if-else

[dreadnaut]

Originally reported on Google Code with ID 243

Starting around line 589, all the way to the end of the main script around line 3500,
is a huge statement

if(!$auth->isAuthorized()) { ... } else { ... }

It has to go at a certain point, and now is a good moment as any other. The main improvement
is increased clarity and a 3KB size reduction.

A 250KB(!) patch against rev 450 is attached below.

Reported by dreadnaut on 2014-01-06 23:02:00

<hr> * Attachment: remove-huge-if.patch

[Christopher Kramer]

Okay. I see advantages and disadvantages.

I would not say saving 3kb code size is a big argument, but okay, it is one.
Improved clarity, yeah, okay.

On the downside: You duplicated the "footer code" which currently is only </body></html>.
I really want to decrease redundant code, not increase it. Not to save file size, but
to improve maintainability.
Imagine we added some additional footer code. We would need to add it at 2 places and
we would probably forget one of them first.
So maybe that's the time to start using some kind of template files.
So I would propose creating a file templates/footer.php that currently is only "</body></html>".
And include it where needed. Of course the build tool needs to replace the include
with the actual file content. As long as we don't have big template files that are
used more than once, the build tool already offers everything we need. Once big templates
are included more than once, we could improve the build tool so it only copies the
code in there once.
What do you think?

Reported by crazy4chrissi on 2014-01-15 09:39:50 - Labels added: Type-Enhancement - Labels removed: Type-Defect

[dreadnaut]

The 'footer code' is longer that </body></html>, but it's only shown in the main page.
The login page, as the help page, doesn't have a footer. There is already another </body></html>
just a few lines before the infamous if.

But since the only reason that the if exists is not to duplicate that echo line, I
think the balance tips toward the removal.

Collecting templates: that would indeed be nice. Should we have many files, or maybe
just a separate module?

A few ideas about possible directions:
- a php script for each 'template', included/embedded by the main script; or
- a single Render class with static methods header($title), tabs(...), footer(...),
or
- a smarter Page class, which might store some data to use in different templates

$p = new Page( array(
 'database' => ...,
 'table' => ...,
 'useful stuff'
) );

$p->title = '...';

$p->header();  // outputs header, using the info stored in the object
$p->viewTabs(/* $current = */ 'view');
// main script does stuff, produces output
$p->footer();

Reported by dreadnaut on 2014-01-16 12:40:47

[Christopher Kramer]

Ok, remove the if and duplicate the echo. (Feel free to commit the change)

Maybe we should discuss templates in another issue, sorry for coming up with this here.
I think I prefer multiple files, one for each thing. It makes development easier compared
to one big file/class/...
But maybe we should introduce a template class that currently only handles the include.
In case we decide for another solution, we could just change the class and would not
need to change all places where templates are used.

I don't think we need a big template class, especially nothing like a template framework
like smarty. phpLiteAdmin should be small.
But we can discuss how we want to do this in detail.

What I come up with based on your idea:

$p = new Page();
$p->setVar('title','...');

$p->template('header');
$p->template('viewTabs');
$p->footer();

and the Page class (currently) more or less only includes the template files:

class Page {
private vars=array('title'=>'phpLiteAdmin');
public function setVar($var,$val) { $this->vars[$var]=$val; }
public function template($name) { $templateVars=$this->vars; include('templates/'.basename($name).'.php');
}
}

Reported by crazy4chrissi on 2014-01-16 13:06:24 - Status changed: Accepted

[Christopher Kramer]

$p->template('footer');
instead of $p->footer();

Reported by crazy4chrissi on 2014-01-16 13:07:52

[dreadnaut]

extract()¹ could also be useful in this case:

public function template($template_name) {
  extract($this->vars);
  // $title and other contents of $this->vas are now defined in the current scope
  include('templates/' . basename($template_name) . '.php');
}


[1] http://php.net/extract

Reported by dreadnaut on 2014-01-16 13:19:26

[Christopher Kramer]

yes, good idea.

Reported by crazy4chrissi on 2014-01-16 13:40:25

[dreadnaut]

Original issue (the long if) fixed with rev457.

Unfortunaly I misspelled the issue number in the commit message, sorry about that :|

Reported by dreadnaut on 2014-01-17 11:38:25 - Status changed: Fixed