Getting private values from the outside

[Acs]

Hey!

 

I have a config class in my small framework, where all the properties are private.

I had a get method which I would call to, obviously, get the data from the private properties.

But one day I had the idea to use __get to retrieve the values by just calling $config->propertie instead of $config->get("propertie")

 

I only do it this way to make sure all the data in the config class is unchangeable at runtime and I can easily  access it.

 

I know this is not the greatest use of the __get magic method, but is it that horrible? It's just for this special config class

 

What do you guys think?

[corbin]

In the OOP ideology world, __get and __set are evil.  Straight up evil.  Well, kind of.

 

__get entirely breaks the idea of encapsulation.  Why declare variables as private if you can just get them through __get()?  Anyway, I'm sure someone better versed in OOP than I will come along.

 

 

Oh, I should note that I'm guilty of using __get() too.

[Acs]

Well it is useful to use __get in this situation. I can get the contents of the private properties, but I know that they cannot be changed.

[448191]

__get and __set are not evil per se. They're easy to misuse though.

 

As a rule, don't allow direct access on private data in these methods. Use accessor methods (which you may already have), delegate to those.

[Acs]

Yes, as I said I had the get method, but I just started to use __get because it was simpler to just call the properties instead of calling the method to call the properties. I know that basically that is still what I do, since __get is a method and that method is called when I do $config->propertie, but this way it just looks nicer, I guess.. no real reason to do it this way.. it's just because I can and it looks nice

[448191]

If the data is meant to be accessible, there is no harm in allowing access through __get. If you have an accessor method, delegate to it. If you'd rather just have __get, and no regular accessor method, that's fine too.

[dbo]

I guess I'm "old school" but I always use accessor and mutator functions rather than any of this __set/__get crap. It's slightly more verbose making it easier to read IMO.

[448191]

When you only have a __get method, it is effect a accessor method. As long as you treat it as that, I don't see any problem with it. Personally, if I have one, I have an accessor method too, to which I delegate.

[dbo]

Makes sense. And you're smarter than me. That's why you have a bunch of blue stars and I only have a bunch of gold stars

[448191]

No, I think I have blue stars because I've been around longer and more active. Doesn't make me smarter by any measure.

 

Also, I proposed you for those pretty yellow thingies. So I guess that means I do think you are pretty smart, don't you agree?

[dbo]

Hehe. I was just making a joke, but all kidding aside you are much better than I when it comes to the nitty gritty details on various design patterns and a lot of best practices. While I try to take these things into account I typically take the route that seems the most logical to me. In general, I just try to write code that is pretty readable so that it's easier to maintain downstream.

 

Thanks for the nomination! I hope that you're still happy with your decision.

[corbin]

Hrmmm I had always heard that __get was bad practice.  Guess it's just the implementation of it that can be bad.

[Liquid Fire]

Like someone else already said, __get/__set breaks encapsulation which begs the question why not make it public and remove the overhead.

 

Accessor functions i think is that way to go.  This allows you to make sure data that is passed it correct and perform extra functionality if needed.

[Acs]

Because if they are public they can be changed and I don't want that. I just want to access them and not be able to change them

[Minty]

If were to use accessor methods I would use __call to automagically define any getVar (where Var is any variable) and return it's value.  This protects protected properties and allows other checks that the data should be accessible.

[dbo]

I more or less just do config classes like this. The main thing is having any data that is going to be used multiple places easily available. While this solution certainly isn't anything elaborate, that's what makes it powerful in my eyes. If you keep your feature set simple, easy to use, and easy to read (maintain) it will allow you to add on and grow as necessary. If you're looking for a more elaborate implementation make sure your needs warrant it. Complex systems are often harder to maintain longterm.

 

class Config
{
   public __construct() { }

   public getValue($key)
   {
      $arr = array();
      $arr['username'] = "user";
      $arr['token'] = "1352sdcvdstr4";

      if( isset($arr[$key]) ) { return $arr[$key]; }
      return "";
   }
}

$config =  new Config();
echo $config->getValue('token');

 

 

[Acs]

You are doing the same thing I am doing... but using __call

[corbin]

The config class I use has an accessor method.  It's like this:

 

<?php

class Config {

private $settings = array();

public function __construct() {
	$this->settings = parse_ini_file('includes/Config.ini', true);
}

public function Get($name) {
	if(strpos($name, '::') === false) {
		if(array_key_exists($name, $this->settings)) {
			return $this->settings[$name];
		}
	}
	else {
		$parts = explode('::', $name);
		if(array_key_exists($parts[0], $this->settings)) {
			if(array_key_exists($parts[1], $this->settings[$parts[0]])) {
				return $this->settings[$parts[0]][$parts[1]];
			}
		}
	}

	$trace = debug_backtrace();
	trigger_error(
		'Undefined property: ' . $name .
		' in ' . $trace[0]['file'] .
		' on line ' . $trace[0]['line'],
		E_USER_NOTICE);
	return null;
}

public static function GetInst() {
	static $inst = null;
	if($inst == null) {
		$inst = new Config;
	}
	return $inst;
}

}

$db = new CDB(Config::GetInst()-Get('db'));
//or to get just the DB host:
echo Config::GetInst()-Get('db::host');

 

 

(Yes, I realize I could abstract out the parsing and easily make it not bound to ini files.)

 

(Disclaimer:  I think I may have stolen this from somewhere a long time ago and modified it, so if it looks familiar, that's why.)

 

 

 

 

 

 

 

 

Oh, I have a question.  Earlier you said you don't want the vars public because then they could be changed.  Why would anyone ever want to change them?

[448191]

Like someone else already said, __get/__set breaks encapsulation which begs the question why not make it public and remove the overhead.

 

Accessor functions i think is that way to go.  This allows you to make sure data that is passed it correct and perform extra functionality if needed.

 

And why, may I ask, do these magic methods break encapsulation more than accessor methods?

 

Yes, you can misuse __get and __set, and break encapsulation. But this is NOT a property inherent to these magic methods, it's programmer fault.

 

If __get and __set delegate to my accessor (mutator) methods, it doesn't break encapsulation any more than my accessor methods. If I only have a __get method, and no accessor method (not recommended), it doesn't break encapsulation, as long as I code defensively.

 

Really. Can people please stop simply echoing others?