Is there a neat,clean way to avoid code repetition like this?

[MelodyMaker]

Hi, I would like to know if there's a way to avoid bad repetition like this one:

I have a User class:

class User{
        private $conf;

private $photos 		= Array();
private $addresses 	= Array();
private $descriptions 	= Array();
        ...
        public function __construct($userData){		

		$this->conf			= Configuration::getInstance();
                        ...
        }
        
        public function addPhoto(Photo $photo){		
	$photoNumber = $this->getPhotosAmount();

	if( $photoNumber < $this->conf->userMaxPhotos){
		$photo->setIdUser($this->u_idUser);			
		$photo->setNumber(++$photoNumber);
		$this->photos[] = $photo;
		return true;
	}
	return false;
}

public function addDescription(Description $description){		
	$descNumber = $this->getDescriptionsAmount();

	if( $descNumber < $this->conf->userMaxDescriptions){
		$description->setIdUser($this->u_idUser);			
		$description->setNumber(++$descNumber);
		$this->descriptions[] = $description;
		return true;
	}
	return false;
}

public function addAddress(Address $address){		
	$addressNumber = $this->getAddressesAmount();

	if( $addressNumber < $this->conf->userMaxAddresses){
		$address->setIdUser($this->u_idUser);			
		$address->setNumber(++$addressNumber);
		$this->addresses[] = $address;
		return true;
	}
	return false;
}	


        ...


}

 

 

 

Of course, there are also three "remove" functions. It looks like very bad, to me...

As you can see, the methods are very similar and this is very annoying.

So, I decided to create and abstract class called "profileItem" which is a superclass of Photo, Description, and Address Class.

 

The abstract ProfileItem Class:

 

abstract class AbsProfileItem{

private $number		= 1;
private $idUser 	= 0;

public function getIdUser(){
	return $this->idUser;
}
public function setIdUser($idUser){		
		$this->idUser = $idUser;				
}	
public function getNumber(){
	return $this->number;
}
public function setNumber($number){		
	if($number > 0){	
		$this->number = $number;
	}				
}
}

 

The Photo class:

 

class Photo extends AbsProfileItem {

private $url		= "";
private $caption	= "";	

public function __construct($url,$caption){

	$this->url 			= $url;
	$this->caption 		= $caption;			
}

public function getUrl(){
	return $this->url;
}
public function setUrl($url){
	$this->url = $url;
}

public function getCaption(){
	return $this->caption;
}
public function setCaption($caption){		
	$this->caption = $number;				
}  

}

 

Address and Descriptions are similar...

 

The problem is that I don't know how to integrate them with the user class...I mean, I need, for example an addItem method which can be used for Description, Photo, and Address.

But at some point, there will be a conditional statement, something like:

if ($item instance of "Address") then "the address should be added to $this->addresses

if ($item instance of "Photo") then "the photo should be added to $this->photos

if ($item instance of "Description") then "the description should be added to $this->descriptions

...

 

Is there a clean, elegant way to do this (maybe with the help of a pattern...don't know)?

 

Thank you very much in advance!

 

Davide

 

 

 

 

 

 

 

[keeB]

Well, I guess now you know why we kept telling you to take these methods out of the User class where they do not belong

 

Address and Descriptions are similar...

 

The problem is that I don't know how to integrate them with the user class...I mean, I need, for example an addItem method which can be used for Description, Photo, and Address.

But at some point, there will be a conditional statement, something like:

if ($item instance of "Address") then "the address should be added to $this->addresses

if ($item instance of "Photo") then "the photo should be added to $this->photos

if ($item instance of "Description") then "the description should be added to $this->descriptions

...

 

Is there a clean, elegant way to do this (maybe with the help of a pattern...don't know)?

 

That would be terrible and illegible. What you're looking, honestly, is to create a few Model[1] Objects.

 

<?php

class UserPhotos {
   private $description = null;
   private $photo = null;
   private $address = null;

   public function setAddress($address) {
       $this->address = $address ;
   }

   public function addPhoto(Photo $photo) {
       $this->photos[] = $photo; //only on display should you care about limiting the amount of photos a user can have, or when you try to save. The responsibility doesn't need to be here.
   }

   public function setDescription(array $description) {
        $this->description = $description;
   }

}

 

[1]: http://en.wikipedia.org/wiki/Model_Object

[MelodyMaker]

Thanks keeb, for your help. The fact is that I see Photos, Description and Addresses as aggregates of a user. I mean, I think they're related each other, or I 'm wrong?

[keeB]

In that photos belong to a user? Sure. But they don't belong in the same object, nor does the handling of them.

[MelodyMaker]

I'm sorry, I don't want to bother you with my questions but a User has:

 

-a name

-an email

- a country ID

...

 

as private properties :

 

Class User{

 

private name;

private email;

private countryID;

...

 

}

 

so, why it should not contain an array of photos and addresses as properties as well?

Is it because they are arrays?

 

 

 

 

 

[DarkWater]

Photos and users are separate entities.  They're related logically, because the user owns them, but not in structure when using them in OOP.

 

On the other hand, the address can be directly related to the User.

[keeB]

I'm sorry, I don't want to bother you with my questions but a User has:

 

-a name

-an email

- a country ID

...

 

as private properties :

 

Class User{

 

private name;

private email;

private countryID;

...

 

}

 

so, why it should not contain an array of photos and addresses as properties as well?

Is it because they are arrays?

 

 

 

 

 

 

I didn't say a User could not have Photo's, I said that operations and data associated with photos should belong to photos.

[MelodyMaker]

So the operations such as:

 

-add

-remove

-move (by "moving", I mean that the photo number 7 can be moved to position 1 or 2 and the other photos (their numbers) must be ordered consequently.

-getPhotoByNumber (which returns a Photo instance, by passing it his number)

 

Should be left out of the user class?

 

But then, should the user class store an array of photos or not? Since Photos are associated to User by a idUser property, they can be left out. Or am I wrong? From a logical point of view it seems also correct to me to store them in the user object since a foto belongs to a user...

 

 

[KevinM1]

Maybe I'm looking at this wrong, but isn't this a clear case of where delegation should be used?  It seems like the most obvious solution to me.

 

I mean, MM wants a User to own (contain) Photos associated with them.  This User, logically, should be able to modify their own Photos.  Why not just have an array of Photos as a property of the User class, then a series of methods that delegate to the Photo's methods?  Something like:

class User
{
   private $photos = array();
   .
   .
   .

   public function addCaption($key, $caption)
   {
      $this->photos[$key]->addCaption($caption);
   }
}

class Photo
{
   private $caption;
   .
   .
   .

   public function addCaption($caption)
   {
      $this->caption = $caption;
   }
}

$Bob = new User();
.
.
.

$Bob->addCaption("vacation", "My summer vacation"); //remember, array keys (in this case, "vacation") can be strings

[MelodyMaker]

I think you're right for the method that changes the caption. But for adding and removing photos?

[keeB]

Why do you need to add/remove photos? Just set the photo list once it's been open:

 

<?php

class User {

   private $photos = null; // this could also be PhotoList (or Gallery) if you want to have operations done on an entire set of photos. That's what I would recommend.
   private $userId = null;

   public function setPhotos($photos) { $this->photos = $photos; }
   public function getPhotos() { return $this->photos; }

}
?>