The GOL website source is now open to everyone

Thanks for the comments folks, constructive chat is appreciated:

Quoting: GoboSmall bug report here as I didn't want to sign up at GitLab after having a quick peek at your code: when you set $year in calendar_ical.php you seem to have borked the google bot path with the empty content. As of right now calling calendar_ical.php without setting the year parameter will most likely invalidate your SQL later on. I guess in case of an empty $_GET['year'] you might want to set $year = date( "Y" ) just like the first branch.

That is now fixed from levi's pull request.

Quoting: wojtek88I have to say that I see very huge difference between code in gamingonlinux/includes/hashids directory while comparing with other code of the system. Object-oriented approach looks much better from my perspective, and I would follow it in the rest of the code.

That's not my code, as you can tell from the head of the file. It's also not used any more, so can probably be removed.

Quoting: wojtek88What's more - I just took a look on one random file and I found commented code (gamingonlinux/includes/crons/itch_games_import.php). I would strongly recommend that you avoid leaving such a stuff in the code - it makes the code very hard to read and it gives no value. If you want to see old portion of business logic, you should find it in the VCS. Leaving it in code is pointless.

I agree, the small few lines were left in while it was being worked on before, simply forgot to remove. Have now done so.

Quoting: wojtek88And the general note - I see that you tend to use magic numbers and values that have some meaning for you, but it's very hard to find out from the code their meaning while looking at one file...

You raise a good point there, I have set one up in that file so it makes more sense :). For some reason I never think to use constants, so that was a nice reminder.
Edit: To clarify this a bit, I actually do use them, but they are generally configs set in the database. Stuff like that though, should just be in the file itself since it's only used for that one place (which is what I have done now in this case).

Quoting: wojtek88Don't get me wrong Liam, I'm very pleased you've opened the code of the site. I just wanted to share with you my comments, You can do with them whatever you want, I just want to share with you my remarks. Maybe you have your reasons to have different opinion, so we can argue for a moment

It's fine, part of the reasoning for opening it up was so people can point out places that need cleaning that I've likely long forgotten about! It's also quite difficult when it's 99% me alone doing it to not only remember to get things done, but also have time to actually research best practices vs actually fixing issues and putting in needed features to make actually running it easier.

Thanks everyone!


Last edited by Liam Dawe on 23 January 2017 at 7:24 pm UTC

Let me add something more Liam:

In general I can see that the code you have is not layered, it looks like every file can do everything -> from accessing request values and producing the output (frontend layer candidate) through manipulating data and calculating things (business logic layer candidate) to accessing database (data access layer candidate).

Personally I would advise strong refactor, because otherwise you won't be able to do fixes in reasonable time. Of course you don't have to start with huge architectural changes, but maybe you can start for example with smaller, per file optimizations.

Quoting: liamdawe

Quoting: wojtek88I have to say that I see very huge difference between code in gamingonlinux/includes/hashids directory while comparing with other code of the system. Object-oriented approach looks much better from my perspective, and I would follow it in the rest of the code.

That's not my code, as you can tell from the head of the file. It's also not used any more, so can probably be removed.

The problem is that the code I referred to is in my opinion the best in the project ;).
You use imperative programming approach in each of the files, you avoid functions, you do not use object-oriented functionalities of the language. I would propose that you start to refactor in small steps.

For example I would start with avoiding so much if-else coding. Let take for example file gamingonlinux/admin_modules/add_article.php
There is a piece of code:
if (isset ($_GET['error']))
{
  if ($_GET['error'] == 'empty')
  {
    $core->message('You have to fill in a title, tagline and text!', NULL, 1);
  }

  if ($_GET['error'] == 'categories')
  {
    $core->message('You have to give the article at least one category tag!', NULL, 1);
  }

  else if ($_GET['error'] == 'shorttagline')
  {
    $core->message('The tagline was too short, it needs to be at least 100 characters to be informative!', NULL, 1);
  }

  else if ($_GET['error'] == 'taglinetoolong')
  {
    $core->message('The tagline was too long, it needs to be 400 characters or less!', NULL, 1);
  }

  else if ($_GET['error'] == 'shorttitle')
  {
    $core->message('The title was too short, make it informative!', NULL, 1);
  }

  else if ($_GET['error'] == 'toomanypicks')
  {
    $core->message('There are already 3 articles set as editor picks!', NULL, 1);
  }

  else if ($_GET['error'] == 'noimageselected')
  {
    $core->message('You didn\'t select a tagline image to upload with the article, all articles must have one!', NULL, 1);
  }
}
Because you use imperative approach, maybe you can proceed with approach that uses functions (one step forward in going to object-oriented world). Don't you think that this piece of code is easier to read and easier to maintain?
function createMessageMapping() {
    $mapping = [];
    $mapping['empty'] = 'You have to fill in a title, tagline and text!';
    $mapping['categories'] = 'You have to give the article at least one category tag!';
    $mapping['shorttagline'] = 'The tagline was too short, it needs to be at least 100 characters to be informative!';
    $mapping['taglinetoolong'] = 'The tagline was too long, it needs to be 400 characters or less!';
    $mapping['shorttitle'] = 'The title was too short, make it informative!';
    $mapping['toomanypicks'] = 'There are already 3 articles set as editor picks!';
    $mapping['noimageselected'] = 'You didn\'t select a tagline image to upload with the article, all articles must have one!';
}

$key = $_GET['error'];
$messageMapping = createMessageMapping();
if (isset($key)) {
    $value = $messageMapping[$key];
    $core->message($value, NULL, 1);
}
P.S. In general I think mixing functions and imperative logic in one file is not a great idea. You can divide it to 2 files (one with function declaration one with logic) or proceed to object-oriented approach (which would be the best I guess, but still, I don't code in PHP so much so maybe more experienced PHP developers can tell more regarding this topic).

Let me know if you want to have such a remarks in this topic and if you are open to code quality improvements in general.

While I appreciate it, this is probably the sort of thing to create issues on the github for.

That is one specific area I have been drafting up ways to improve, including using functions.

Respect for going opening the source! You continue to do the work of the lawd, Liam. Also it's cool to see some love for GitLab over GitHub.