search

Buzzinoffbond / phpliteadmin

Automatically exported from code.google.com/p/phpliteadmin
0 stars 0 forks source link

Remove the 3000-lines long if-else #243

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
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.

Original issue reported on code.google.com by dreadnaut on 6 Jan 2014 at 11:02

Attachments:

GoogleCodeExporter commented 9 years ago 
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?

Original comment by crazy4ch...@gmail.com on 15 Jan 2014 at 9:39

GoogleCodeExporter commented 9 years ago 
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();

Original comment by dreadnaut on 16 Jan 2014 at 12:40

GoogleCodeExporter commented 9 years ago 
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'); }
}

Original comment by crazy4ch...@gmail.com on 16 Jan 2014 at 1:06

GoogleCodeExporter commented 9 years ago 
$p->template('footer');
instead of $p->footer();

Original comment by crazy4ch...@gmail.com on 16 Jan 2014 at 1:07

GoogleCodeExporter commented 9 years ago 
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

Original comment by dreadnaut on 16 Jan 2014 at 1:19

GoogleCodeExporter commented 9 years ago 
yes, good idea.

Original comment by crazy4ch...@gmail.com on 16 Jan 2014 at 1:40

GoogleCodeExporter commented 9 years ago 
Original issue (the long if) fixed with rev457.

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

Original comment by dreadnaut on 17 Jan 2014 at 11:38