In this final post in the series about my interview with X-Team, I will write about how it ended and will summarize my experience. Also, I will explain why I decided to dedicate so many posts about an interview with a company, which actually failed for me (not my fault!).
- The first answer to a x-team and my career story
- The second answer to a x-team and my career story
- The third answer to a x-team with tech details
- My comment to a “The value of the team” blog post
Input
So, I have failed, and it happened very unexpectedly for me with a ridiculous reason. And why I want to write about what happened? It will not change anything and it happened a long time ago, so it doesn’t even matter. But the task to write about that was added in my blog posts queue and I want to accomplish it. :) Also, I want to talk here about what is wrong with interview process in X-Team and in general – in IT industry. An experience with X-Team would be just an example.
I had a pretty long email conversation with X-Team talents team and they asked me to write a lot about myself and my experience. And I have shared what I have written in the previous posts. Despite I spent some time and effort on writing, they weren’t fast in their answers to me. Sometimes I had to wait for one week for an answer.
Anyway, we reached a step when they gave me a simple task. It was a PHP file from some legacy code system with very-very bad code in it. I was wondered why they gave such bad code, because usually when I look at such code – my eyes are bleeding. :) Since I haven’t signed any NDA I will post this file here.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 |
<?php /** * Social network site where you can see a list of users and "follow" them. */ // setup the `Db` object: require_once 'lib/db.php'; // setup the `Session` object: require_once 'lib/session.php'; if (!Session::isLoggedIn()) { throw new Exception('You must be logged in to view this page'); } // display a user as html function displayUser ($user, $tag='div') { $full_name = $user->first_name . ' ' . $user->last_name; // get user's avatar from 3rd-party avatar service, based on which environment we're running in if (strpos($_SERVER['SERVER_NAME'], 'localhost') !== false) { $avatar_data = json_decode(file_get_contents('http://dev.avatars.com/user/' . $user->id)); } else { $avatar_data = json_decode(file_get_contents('http://www.avatars.com/user/' . $user->id)); } ?> <<?= $tag ?> class="user"> <a href="/users/<?= $user->id ?>"> <img src="<?= $avatar_data->url ?>"> <?= $full_name ?> </a> <a href="?follow=<?= $user->id ?>">Follow</a> </<?= $tag ?>> <?php } // check that a user ID is valid function validateUserId ($id) { if (!is_numeric($id)) { // log invalid user IDs to help us track down bugs. error_log("Invalid user id: {$id}"); return false; } return true; } // load the user's custom theme config $theme_filename = dirname(__FILE__) . 'themes/' . Session::getLoggedInUser()->theme_id . '.php'; if (file_exists($theme_filename)) { require $theme_filename; } // show all users if (isset($_GET['show-all'])) { echo '<ul>'; foreach (Db::getUsers() as $user) { displayUser($user, 'li'); } echo '</ul>'; } // show a single user else if (isset($_GET['id'])) { $id = $_GET['id']; validateUserId($id) or throw new Exception('Please select a valid user ID'); $user = Db::getUserById($id); if (!$user) throw new Exception('Sorry, user not found'); displayUser($user); } // follow a user else if (isset($_GET['follow']) { $id = $_GET['follow']; validateUserId($id) or throw new Exception('Please select a valid user ID'); $user_to_follow = Db::getUserById($id); if (!$user_to_follow) throw new Exception('Sorry, user not found'); $current_user = Session::getLoggedInUser(); try { $current_user->follow($user_to_follow); } catch (Exception $e) { throw new Exception('Unable to follow at this time'); } // inform the user that they have a new follower. mail( $user_to_follow->email, 'You have a new follower!', "{$current_user->username} is now following you.", 'From: notifications@awesome-social-network.com' ); // also update the activity feed, so users can see what others are doing on the site. Db::updateActivityFeed(array( 'action' => 'follow', 'from' => $current_user->id, 'to' => $user_to_follow->id, 'timestamp' => time(), )); echo 'Followed! <a href="/">Return to home</a>'; } ?> |
My task was to describe and explain how to solve issues in this shit code. So, I was very confident in myself, because I thought that I need to talk about how bad this code. And I thought that I need to show my view on how good code shall look like: clean, readable, testable, well designed with oop and patterns, secure and etc. I have added a lot of marks.
My submitted result.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 |
<?php //>> issue: all this example looks like one big issue. my eyes were bleeding while code review. // I've even felt a shame while reviewing it. why did you do it with me? :D // solution: the best solution here is to rewrite all project using some framework. /** * Social network site where you can see a list of users and "follow" them. */ //>> issue: use of require method for including files instead of namespaces with PSR autoloading // solution: do code refactoring. add a namespace to every class and use composer with //PSR autoload configuration. // setup the `Db` object: require_once 'lib/db.php'; // setup the `Session` object: require_once 'lib/session.php'; //>> issue: possible issue is exception handling. if no handler registered then the user will see //raw errors output in a browser. it's a security vulnerability and not user-friendly. // solution: check if error handler for displaying errors and exceptions is registered. // if not, register a handler with some HTML design output for errors if (!Session::isLoggedIn()) { throw new Exception('You must be logged in to view this page'); } //>> issue: a mix of HTML with PHP (oh my god...). the code looks like a mess without separation between //business logic and representation into view. // solution: create own minimal implementation of MVC or use some external libs to reorganize code into MVC // with controllers, views, and models. //>> issue: parameter type isn't specified for $user parameter // solution: specify a User object type for a $user param. it will provide help for IDE to suggest auto-completions. //>> issue: phpdoc not used for a function comment // solution: it's better to comment functions with phpdoc with type and description for every parameter // display a user as html function displayUser ($user, $tag='div') { $full_name = $user->first_name . ' ' . $user->last_name; //>> issue: not a good place for env checking, and not a good way // solution: it's better to configure an environment on application bootstrap and save it into //some static config or constant //>> issue: file_get_contents for opening urls. // solution: it's better to use curl extension for http requests. the best way here to write own // lib/http.php with bunch of functions for sending PHP requests or use a library like Guzzle //>> issue: json_decode without parameter check // solution: it's better to store previous function result into variable and do a validation //before use this data in json_decode. use of empty or null data could lead to PHP notice or warning. // get user's avatar from 3rd-party avatar service, based on which environment we're running in if (strpos($_SERVER['SERVER_NAME'], 'localhost') !== false) { $avatar_data = json_decode(file_get_contents('http://dev.avatars.com/user/' . $user->id)); } else { $avatar_data = json_decode(file_get_contents('http://www.avatars.com/user/' . $user->id)); } //>> issue: php with html code mess // solution: a mix of PHP code and HTML output was mentioned in issue above. of it's not possible to use //any MVC here, at least output could be moved to some user.phtml file and be included with passing //a parameter to it //>> issue: user fields aren't validated // solution: what if a $user does not contain field 'url' for example? it could cause a warning or even error. ?> <<?= $tag ?> class="user"> <a href="/users/<?= $user->id ?>"> <img src="<?= $avatar_data->url ?>"> <?= $full_name ?> </a> <a href="?follow=<?= $user->id ?>">Follow</a> </<?= $tag ?>> <?php } //>> issue: functional programming instead of OOP approach. with this approach, functions will not be //organized by their purpose and it will be hard to maintain and scale an application. // solution: the best solution here is to pack connected functions in classes, like User class and etc, // and use methods of classes instead of functions... // check that a user ID is valid function validateUserId ($id) { if (!is_numeric($id)) { //>> issue: destination log file isn't provided. it could lead to a mess inside one big log file //and it will be hard read it and understand something // solution: it's better to use some self-written log system or some ready lib for logging. it's better to //store different errors from different business logic in different files. // log invalid user IDs to help us track down bugs. error_log("Invalid user id: {$id}"); return false; } return true; } //>> issue: next lines of code include some other code, possibly with some functions, in the middle of current PHP script //in this case project became a total mess with uncontrolled relations between files. it's hard to understand include ways //and where which function was used in the project. // solution: the best solution to use namespaces and autoloading. int this case next lines could be packed into some //lib/loader.php into a function. at least all stuff like this would be placed in one place and it would be easier to //find where include was made. // load the user's custom theme config $theme_filename = dirname(__FILE__) . 'themes/' . Session::getLoggedInUser()->theme_id . '.php'; if (file_exists($theme_filename)) { require $theme_filename; } //>> issue: use of global PHP variable as $_GET, $_POST, etc. this variables shouldn't be used in code directly, // because they are global. // solution: get, post could be packed into Request object, cookies and session also to their own object wrappers //and it's better to process all global input in an application entrance point or while app bootstrap and use objects //after //>> issue: and again a mix of PHP, HTML and even with SQL queries. the result of DB query should be provided here //as a parameter and should be validated and processed somewhere above or inside model class // solution: rewrite it into MVC. // show all users if (isset($_GET['show-all'])) { echo '<ul>'; foreach (Db::getUsers() as $user) { displayUser($user, 'li'); } echo '</ul>'; } // show a single user else if (isset($_GET['id'])) { $id = $_GET['id']; //>> issue: we have an error here. we can't use a throw keyword after logical operators. //we could use only expressions here // solution: rewrite it to if/else validateUserId($id) or throw new Exception('Please select a valid user ID'); $user = Db::getUserById($id); if (!$user) throw new Exception('Sorry, user not found'); displayUser($user); } //>> issue: next lines of should be placed into separated controller to process the follow command or into //any FollowProcessor class. now, this script has more than one responsibility and it's hard to maintain. // solution: split logic into two separated classes or controllers or scripts. one will be responsible for a view, //another one for the following a user business logic. //>> issue: error, closing ) is missed // solution: add missing ) // follow a user else if (isset($_GET['follow']) { //>> issue: again errors with OR and global $_GET use // solution: described above //>> issue: we have the same lines of code duplicated for getting a user and it's validation // solution: place this lines of code in function or (better) into UserModel class. $id = $_GET['follow']; validateUserId($id) or throw new Exception('Please select a valid user ID'); $user_to_follow = Db::getUserById($id); if (!$user_to_follow) throw new Exception('Sorry, user not found'); //>> issue: what if at this moment we haven't $current_user or returned type doesn't object but array // solution: validate $current_user first $current_user = Session::getLoggedInUser(); try { $current_user->follow($user_to_follow); } catch (Exception $e) { //>> issue: message from the previous exception will be missed here // solution: $e->getMEssage() should be at leas logged somewhere to see later what happened and //fix it throw new Exception('Unable to follow at this time'); } //>> issue: usage of built in mail function // solution: it's better to use and object wrapper for mailing. // then we will have possibility to change an implementation // inform the user that they have a new follower. mail( $user_to_follow->email, 'You have a new follower!', "{$current_user->username} is now following you.", 'From: notifications@awesome-social-network.com' ); //>> issue: mix of controller code with a model code // solution: all DB operations should be placed inside some related Model or Repository class //and should be used with this class only // also update the activity feed, so users can see what others are doing on the site. Db::updateActivityFeed(array( 'action' => 'follow', 'from' => $current_user->id, 'to' => $user_to_follow->id, 'timestamp' => time(), )); //>> issue: and again mix of php and html. // solution: should be placed into page tpl. echo 'Followed! <a href="/">Return to home</a>'; } //>> issue: PHP script close tag and an empty string after it // solution: it's better to not close php scripts to prevent unreasonable characters output by interpreter ?> |
I have written a lot of marks. My comments with issues and suggestions how to fix them now contains I think twice more lines than the code itself. I was sure that I’m easily passed the test…
But the answer was:
Some things we were concerned about:
– Didn’t see the pros of using well defined object like User
– Forgotten to mention about creating getters for first and last name for user
– Security and performance issues were not detected
What? WAT?
I think it’s worth us reconnecting in 6 months to a year to see how you’ve grown as a developer.
Ahaha, really? Let me explain, what is ridiculous here.
- They have said, that I don’t see the pros of well-designed objects. The Senior Software Developer like me, who for the month have written a lot about his experience with frameworks, patterns, oop and even DDD – doesn’t see the pros of well-designed objects. Seriously?
Conclusion 1. Seems like X-Team has completely different people for different tasks and they do not communicate well with each other. Some guy responsible for a conversation with me, another one responsible for code review. And their reviewer hadn’t even seen all my big emails about my experience. This is only one possible explanation for what happened. - Actually, I have written a lot in my marks in the file about oop, about splitting code into different classes. Just check my notes and you will see.
Conclusion 2. Code reviewer was in a bad mood or he wasn’t attentive enough, he doesn’t even read all my comments? - Let’s go to the next point. Even now after 6 months after it happened I can’t read their notes without a smile. WAT? See 1 and 2, I have written a lot about classes and it’s obvious to have getters and setters when it’s needed.
Conclusion 3. Maybe I just more qualified than their code reviewer. Yeah… Maybe because I’m overqualified they decided to find any reason to say NO to me? - The point about security was the most reasonable, but I will explain it more in the next paragraph.
So, what about security, am I really haven’t noticed security vulnerabilities? Of course, I did notice. And I have written in my comments that, for example, it’s a bad practice to use PHP global variables and about validation. The reason why I haven’t written about security directly because this code is fucking shit. Let me be direct and honest here. I would never agree to work with such bad code. But it’s not the point. The point is that such code is the biggest security issue itself.
In order to make such bad implemented legacy system more secure – every adequate developer would suggest rewriting the project with the use of some modern framework, with modern architecture, libs, and approaches. And the process of redo will fix 90% of possible security holes. Because modern libs and frameworks already taking care of security in its code.
The most qualified developer should be able to use a wide range of components, understand how to combine them together in a flexible architecture. He doesn’t need to take care of most obvious security cases like SQL injections because ORM or QueryBulders takes care of them. Of course in some cases, for example for CQRS, we could use just plain SQL queries.
But it’s obvious, that we would have some framework layer for Request (MVC) and some Validation layer before the query execution. And if we don’t use the framework – any professional developer will write his own simple implementations for such tasks.
This is what’s wrong with this test. This test actually tests nothing, except maybe developer tolerance to the shit code.
If you want to test developer skills – give him some small task to implement from scratch, like converting some excel document in 3-4 different formats, like JSON, XML, CSV, and YAML.
Actually, I had such task from another company (and they were impressed with my solution). And see results. See how he will organize the code, which libs he will use, which patterns he will use. Check how he follow PSR in his code formatting and how he uses SOLID principles. This would be the real indicator of the developer qualification.
Even tests with Codility and such algorithm testing platforms, which I don’t like at all because it’s the same as whiteboard code interview, they are even better!
In the end, they have suggested me to grow as a developer. Ahahaha, can’t stop myself smiling. :)
Output
Well, I have answered them a bit emotionally in an email. And I have included their CEO in the recipients because I thought he will pay attention to the interview process and will change something. But he paid attention only to my emotions. It’s a pity, I didn’t want that.
Now I will try to explain it’s better why I was so emotional. Of course, I was angry about wasted time and efforts. And about such ridiculous test and its results.
But whats really touched me is that they have their YouTube channel. And on this channel, they have very cool motivational videos about developers. It’s the best videos about developers which I have ever seen. And I wanted to work remotely with X-Team very much after watching this videos.
Because with such videos they are trying to speculate on developers emotions and attract some of them for hire. And it works, because it worked with me.
Also, one of their main slogan and feature is that they really care about developers and trying to provide the best working environment for their remote developers.
Wow…
And after that, they forced me to have a one-month conversation with them via email. And gave me such ridiculous test with shit code and told me to grow as a developer after that.
So, they touched my emotions first in a positive way with YouTube videos, and after that, they touched my emotions again in a negative way.
I really think that such treatment of highly qualified candidates isn’t good at all. That’s why I was so emotional in my answer.
I have written, that after what happened I think that all their videos – “all these videos from youtube now look like a marketing fuss for me, nothing more”.
And I’m not proud about that, but I’m not sorry for that, I still think the same.
In the end, their CEO has written me an email with recommendations to be more humble and improve my soft skills. I haven’t answered because didn’t want to argue or continue, I felt too bad, I was disappointed. It’s like when you are a child and you see some sweet candy in shining package and you very excited, you open the package, you make your first bite and you realize that candy taste like shit. This is what I felt.
Also, the CEO said after my claim about bad code – ‘a very worst-case scenario any developer might have to walk into’.
NO! If developer walks into and do nothing with such code or produced such code – he must be fired immediately. No excuses, he is not qualified. The qualified developer wouldn’t even agree to work with such codebase.
Conclusion 5. CEO of the tech company doesn’t know anything about tech. He just trusts his code interviewers. I have concluded that from his answer on email, he started to explain to me how security important for his clients and he didn’t analyze the situation himself. Ridiculous.
The main message of all this post is: if a company want’s to attract highly qualified developers to work for them – it must care about tech. Test task is like a program evaluation period, it’s like the first taste of a candy on the tongue – test task must represent the tasks with which developer would work in the company in case if he will be hired.
Additionally, to the feeling of disappointment, I had an impression, that I was overqualified for X-Team. That impression only strengthened my bad feelings about their recommendation to grow as a developer.
And I have written, that maybe they only need some ‘Indian developers’ to solve tasks fast, not right and not great, just fast. They even have ‘Heroes’ program. And I think they called ‘Hero’ a developer who managed to finish something in a very short period of time, before the deadline. Is it really means to be a ‘hero’, when you forced to work extra hours and produce tech debt and shit code in order to meet deadlines which were arranged without asking developers? I don’t think so. Maybe I’m wrong about their ‘hero’ program, it’s only my impressions.
So, if any of mentioned above points true – such company isn’t for me.
And I want to answer on a recommendation from CEO to improve my soft skills. They are good (I haven’t written great because I’m humble). I’m always learning and improving myself. I’m ok.Their tech interview process – isn’t.
Personally, I communicate very well with clients and with teams. Even more, I often lead teams of 2-5 developers in the office or remotely. Is a person with poor soft skills would be able to do this? They are not even tried to logically connect all information provided in previous emails with my last answer. Bad luck.
That’s is the end. I wish every developer adequate interview processes! :)
PS. Now I have moved to Germany and really happy with my new job. German companies care a lot about people and work-life balance. This is right. :)
