Skip to content

Instantly share code, notes, and snippets.

@tinganho
Last active October 12, 2015 07:37
Show Gist options
  • Save tinganho/d61e5afe33e214bd82f2 to your computer and use it in GitHub Desktop.
Save tinganho/d61e5afe33e214bd82f2 to your computer and use it in GitHub Desktop.
Kattis

Overall feedback

I think that the website is a little bit too barebone PHP. I usually work on a framework which defines frames for developers to work on. These frames usually creates a more strict and more structure for a project. More structure means less spaghetti code. It would be great also if the db queries could be handled in a backend service. More modern web framework doesn't even have a DB and should be stateless. I have also seen Javacript code written in the HTML templates, which is not the nicest solution, especially for JS developers like me.

Specifics

file_exists is an IO function and runs on every request. Since you cannot lazy cache it, because of how PHP works. I think you should at least define a map or an array with all the paths.

function delegate_request($controller, $action, $params) {
    $controller_path = CONTROLLERS . $controller . '.php';
    if (!file_exists($controller_path)) {
        internal_error('Could not load controller, file not found.');
    }
    
    require_once $controller_path;
    ...
}

Also require_once is also on IO function. Though I guess we cannot do anything then to change programming language.

CSRF protection

You don't need to have a token to protect from CSRF. Just create a custom HTTP header(X-CSRF) and add a check on all state changing requests POST/PUT etc. More stateless solution. Though requires that forms to be submitted throught JS.

No middleware

No middleware framework that handles the request. It's more or less common to use middleware as a part of the request to build up data and info so that the developer can consume.

CSS class naming

Don't use nested selectors. The querying occurs backwards. It is bad practice to use .component .title instead of .componentTitle or just #componentTitle. The lookup is nearly instant on the latter. But also CSS is an override only language having a generic CSS selector like .title will likely be overriden by an another selector. I have also seen html tags defined in the selectors where it provides little benefit, like div.something, div is already super common why use it to narrow down the query?

CSS styles and dimensions

Please separate dimension CSS properties from styles. While dimensions aren't so reusable styles like fonts and background color are.

No CSS preprocessor

It's a common practice to use CSS preprocessor to deal with bare CSS problems right now. I use Compass/SASS it extends the CSS language with very good language features like DRY media queries and more.

No JS module handling

It's a common practice to use a JS module bundler to let you write JS modules. There are a quite of alternatives like SystemJS/RequireJS.

Don't use view libraries like Bootstrap/jquerui

It is very hard to customize view libraries. It also takes a lot of time. But of you succeed to customize something the customization is sometimes never a clean solution. Like the HTML doesn't have the same semantic meaning as the design.

Don't use gettext

Gettext doesn't deal with problems like one sentence having two plural words. I like 2 cats and 2 dogs. Though I guess there are no better solution for PHP than to stick using gettext.

Don't use text as localization keys.

Since the localization storage is just a key value store. It doesn't make sense to have the value as the key. You can also change the value without changing the key if you have a name that represent the localization string as a key.

Componentize your views

I have seen quite large templates including a lot of components that could be factored out.

Naming of variables and functions

I've seen you are using camelcase in some and underscore in some.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment