Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation, some refactoring and new little feature #7

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Documentation, some refactoring and new little feature #7

wants to merge 8 commits into from

Conversation

AmsTaFFix
Copy link

  • add documentation to classes;
  • change names of arguments on more meaningful;
  • add group_id parameter, when uploading photos in album,
  • etc...

…field

Add group_id because when you want to upload photo in group's album you have groupId in response of photos.getUploadServer, and have to set group_id when call photos.save method
… property to send photos.save request

Because you have to set group_id when uploading photos in group's album
Because album_id/group_id is more meaningful than album/group
…method

Because array ($files) will not be with numeric indexes every time
…:uploadAlbum() method

Because listOfFilePaths is more meaningful then files. When you read new name, you can understand meaning of this arguments without reading documentation and fo not ask question "What files? Names? Paths? Resources? Maybe special objects?"
{
if (sizeof($files) > 5 || sizeof($files) == 0) {
if (sizeof($listOfFilePaths) > 5 || sizeof($listOfFilePaths) == 0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks more complicated than it is. $files was good, just because phpDoc giving all necessary information.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как вы тут описали "phpDoc дает все необходимую информацию", но зачем читать этот phpDoc, когда само название переменной передает ВЕСЬ смысл содержимого в ней. И посмотрев на само название уже можно понять что туда передавать не тратя время на обращение к phpDoc'ам или чего хуже к реализации.

P.S. на данное изменение меня сподвигло маниакальное стремление к понятному коду, при чтении которого все становится ясно без обращения в посторонние источники.

P.P.S Если мои аргументы не произвели никакого действия, я изменю название обратно и впредь буду обсуждать изменения названий аргументов и переменных )

P.P.P.S. только что подошел коллега, который разбирался с кодом вместе со мной, так вот он мне напомнил, что он не понимал что передавать в параметр $files, пока не посмотрел на реализацию метода. И сказал, если бы было название listOfFilePaths или просто filePaths, не пришлось бы лезть в реализацию метода.

@getjump
Copy link
Owner

getjump commented Mar 18, 2015

Seems pretty good, but look at comments that i had made upon commits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants