Given that this is a follow-up question, and I've dealt with some issues here already, I'm not going to be as verbose. I will, however, refer to a couple of my initial critiques, and offer some advice on this code, too. I see several issues, I've listed them below, but some do require some explanation, so if after a huge blob of text, you see a new list item, that means I'm touching on a new subject. Just for clarity, because this answer could do with some refactoring, too :)
Functions return: Functions, or methods for that matter, do not echo anything. Functional entities (functions, methods, and classes, for that matter) return things, they do not echo. The renderEvent method you have is still littered with echo statements.
Classes throw, they don't exit: A class is a single, supposedly portable unit. It should not be able to control the entire flow of the application it is a part of. That's why methods contain no exit calls, your do. DON'T. DO. THAT!. When a method encounters an issue, and the class has no code to deal with such an event, then you have an "exceptional" situation. What any good class then does is throw an exception. For example:
Say I have this method:
public function notGreatButJustAnExample(array $argument)
{
$return = '';//instead of echo
if (isset($argument['images']) && is_array($argument['images']))
{
foreach ($argument['images'] as $k => $image)
{
$return .= ($k+1).' image == '. $image. PHP_EOL;
}
}
if (!isset($argument['important_key']))
{//without this, I can't continue
throw new InvalidArgumentException(__METHOD__.' needs important_key to be set in argument-array');
}
}
I need this key, without it, the method can't do its job. If that key is missing, the data is, too, and the method has received an invalid argument. Hence, the InvalidArgumentException is thrown.
Other exceptions that could be thrown in such a method is when 2 keys need to be arrays that are equally long, for example $argument['images'] and $argument['img_descriptions']. If the two arrays exist, but aren't equally long, then the code that called the method probably contained bad logic, in that case:
if (count($argument['images']) != count($argument['img_descriptions']))
{
throw new LogicException('Number of images does not match number of descriptions');
}
Notifies the user of this problem. Think of exceptions as your class shouting back at whomever calls one of its methods: "If you want me to do so and so, then I expect valid input. If I don't have valid input, then that's your problem". That's why you throw it back to the caller.
only make public what needs to be called: What the user doesn't need, he shouldn't see. Any method that is used internally by the class, shouldn't be callable from anywhere else. The user should not be allowed to call it, so make it protected (or private). I tend to prefer protected, which means that, if every you extend your class, the children will have access to these methods, too. private methods only exist in the scope of the class itself. protected exists in the scope of the class, and all of its children.
Style and standards: Try to adhere to the coding standards as much as possible. That is to say: class names start with an Upper-case char, and their opening brace go on the next line. You wrote class event {.
According to the standards (unofficial, but it's the only one we've got), it should be written like this:
class Event
{
- A S.O.L.I.D. start: The SOLID principles are well known by many. I'm prepared to defend the following statement(s) to the bitter end, though I am aware that this might enrage some people. The most important of all SOLID principles is, IMHO, SRP. The Single Responsability Prinsiple. These principles aren't just for show. A lot of good practices and principles sound great in theory, but the SOLID principles tend to be quite easy to follow, especially the SRP. Not only are they easy to apply, they will in fact help you write better, more generic code quite easily.
I've always been skeptical about "ultimate mega-super-unbeatable-design theories", believe you me. But these 5 principles are just simple, common-sense ideas put together. What is wrong with common sense?
If you answered nothing to my rhetorical question, you should realize 2 things: First, you don't answer to rhetorical questions and second: Your class violates the SRP quite blatantly. It connects to the DB and fetches data (1 task/reason to change), formats it (another task/reason to change) and takes care of the output (another task/reason to change). That's 3 distinct responsibilities. Ideally, you'd use 3 classes for this, but given PHP's suitability as a templating language, 2 will do, too (the output part can be a simple PHP+HTML script, containing no logic other than the occasional loop and if-else.
- "What's in a name?": That, which you call
event, by any other name would work as good... Or something. Well, when writing code, there's a lot in a name, especially when classes are involved.
Allow me to explain myself:
Good code is self-documenting. A class name, and its methods tell the user a lot about what it's for. Your class is called Event. If I see a line of code that reads new Event(), I expect it to create a data model, an instance that describes an event of sorts.
Once I notice I can pass it instances of DateTime, I'll assume the instance describes an event that can be recurrent, or stretch out over a period of time. I'd also expect to be able to pass strings (descriptors) and perhaps an array of attendees to this object.
This data itself should be gotten via an EventMapper instance, which does the DB work. At least, I'd expect to see something of the sorts. This mapper could have methods that return Event instances, depending on arguments (for example: 2 dates, returns an array of all events that take place in that date range).
These Event instances could then move on to an EventRenderer class, which has a getOutput or render method. The constuctor could take a value (preferably predefined constants) that set the output mode I'd like. When I call this render method, I'd expect it to return something that I can echo, if I so desire. I might choose to write the output to a file or DB, too. That's up to me, not up to the class to decide. Hence: method's don't echo. Here's an example:
class RenderEvent
{
const RENDER_JSON_MODE = 1;
const RENDER_HTML_MODE = 2;
const RENDER_XML_MODE = 4;
const RENDER_ALL_MODES = 7;
protected $mode = null;
public function __construct($mode = self::RENDER_HTML_MODE)
{//default
$this->mode = $mode;
}
/**
* Takes an event, renders it according to the set mode
* @param Event $event
* @return string
*/
public function render(Event $event)
{
if ($this->mode === self::RENDER_ALL_MODES)
{//return assoc array, use constants as key
return array(
self::RENDER_JSON_MODE => $this->setOutputMode(self::RENDER_JSON_MODE)
->render($event),
self::RENDER_HTML_MODE => $this->setOutputMode(self::RENDER_HTML_MODE)
->render($event),
self::RENDER_XML_MODE => $this->setOutputMode(self::RENDER_XML_MODE)
->render($event),
);
}
if ($this->mode === self::RENDER_JSON_MODE)
return $event->toArray();//or something
$dom = new DOMDocument();
if ($this->mode === self::RENDER_HTML_MODE)
{
$dom = new DOMDocument;
$table = $dom->createElement('table');
//build dom
return $dom->saveHTML();
}
//no if required, we're rendering XML once we get here
//very similar to render HTML, but:
return $dom->saveXML();
}
/**
* Change output mode
* @param int $mode
* @return EventRenderer
* @throws InvalidArgumentException
*/
public function setOutputMode($mode)
{
if ($mode === self::RENDER_ALL_MODES)
$this->mode = $mode;
else if (($mode & self::RENDER_ALL_MODES) === $mode)
$this->mode = $mode;
else
throw new InvalidArgumentException('Use constants for modes, '.$mode.' is not a valid output mode');
return $this;
}
}
This code allows the user to use your events in ajax calls, or NoSQL data storage situations:
$event = new Event();//<-- assume everything is set up
$renderer = new EventRenderer(EventRenderer::RENDER_JSON_MODE);
$json = $renderer->render($event);
$mongo->save($json);
//or in AJAX call context:
echo $json;
As you can tell, I have the choice here: echo or pass the rendered data. That makes this code a more likely candidate for re-use.
type-hints: You've used 1 type-hint. That's all fine and dandy, but I'd like to see you use type-hints as much as possible. The first method takes one argument $Array, but what if I were to pass a string? change the signature to public function renderEvent(array $Array), to prevent such issues.
Markup is structured, strigs are not: stringing together markup is un-maintainable. As you start getting the hang of OOP, you'll perhaps one day want to extend this class. Having it string together markup will mean you'll have to override any method that does so, in order for it to work in the child's context. That's not ideal.
Add to that the fact that stringing together markup is a delicate procedure, and once a bug rears its ugly head, it's a nightmare to debug.
Using a DOMDocument instance allows you to create nodes, hold references to them, and add, remove and move them about in the dom as you please. Ensuring valid markup every time. Chec the documentation for DOMDocument and all its related classes. It's a bit tricky to get your head round at first, but it is worth it.
Now, mysql_*. You've commented on my other answer, saying you are aware of mysql_* being deprecated, but the project is already using this extension. Pardon my french, but that's utter bulls***.
If the existing code is using a deprecated an (hopefully) soon to be removed extension, the last thing you should be doing is adding new code that makes that very same mistake. You'll have to refactor the existing code at some point in the future. By adding more code that will need to be refactored, the only thing you're doing is increasing the amount of code that will need to be re-written.
Do I mean that you should refactor the entire project before you start adding code? Not necessarily. But if you are going to add new features/code, then at least write them in such a way that the new code doesn't require refactoring! That's common sense.
I always like to use extensive analogies to explain what I'm saying, so humour me and let me give you a weird analogy for this, too:
Say you have a house, it's not brand new, but it's home. There are a couple of doors that squeek and the carpets could do with some cleaning. Due to circumstances, you've had to spend your savings on an annexe you're building. Would you fit that annexe with dirty, old, worn out carpets, or would you fit new, nicer carpets there first? Of course you'd go for the new carpets, and you'd start saving up to fit new carpets in the rest of the house as soon as possible.
What you're doing is the exact opposite. You're writing new code (ie building an annexe), but you're fitting it with the same old, worn out and scruffy carpets. That kind of takes the shine away, doesn't it? And to add insult to injury, you'll have to save up some more, because when you want to refit the carpet, you'll have to buy more, and there's going to be more work that needs doing, too. Save yourself the pain of having to do twice the work: whatever code you add, make sure it's future-proof. Meanwhile, work on ditching that mysql_* from the existing code-base.
You can do this quite easily now that you've decided to go with an OO approach. The great thing about your using classes is that a class needn't care about anything that is going on outside of itself. It can hold its own DB connection (not create it itself, but rather _inject it as a dependency => SOLID), using whatever extension you (as its author) choose. I'd suggest you provide your class with an up-to-date db connection, and think about refactoring the rest of the project in due time.
Whatever you do do not increase the amount of code you'll need to refactor by simply adding mysql_* code. Whatever new code code you add should use the new extensions. The old code should be re-factored, or factored out.
Some other niggles
The code you have now still suffers from the same injection vulnerabilities I pointed out in my initial answer. Other issues still exist, too, like your creating new DateTime instances too many times. The recur* methods, too, could do with a bit more work.
As luck would have it, I've written a substantial answer that is very much related to what you're doing just yesterday. Check that out, and refactor the recurrency methods accordingly.
