Duplicate code is one of the most pungent code smells. A rule that is often used is to re-structure code once it is duplicated in three or more places.
Common duplication problems, and corresponding solutions are:
Complex classes like VirtualRESTServiceClient often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.
Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.
While breaking up the class, it is a good idea to analyze how other classes use VirtualRESTServiceClient, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 46 | class VirtualRESTServiceClient { |
||
| 47 | /** @var MultiHttpClient */ |
||
| 48 | private $http; |
||
| 49 | /** @var array Map of (prefix => VirtualRESTService|array) */ |
||
| 50 | private $instances = []; |
||
| 51 | |||
| 52 | const VALID_MOUNT_REGEX = '#^/[0-9a-z]+/([0-9a-z]+/)*$#'; |
||
| 53 | |||
| 54 | /** |
||
| 55 | * @param MultiHttpClient $http |
||
| 56 | */ |
||
| 57 | public function __construct( MultiHttpClient $http ) { |
||
| 60 | |||
| 61 | /** |
||
| 62 | * Map a prefix to service handler |
||
| 63 | * |
||
| 64 | * If $instance is in array, it must have these keys: |
||
| 65 | * - class : string; fully qualified VirtualRESTService class name |
||
| 66 | * - config : array; map of parameters that is the first __construct() argument |
||
| 67 | * |
||
| 68 | * @param string $prefix Virtual path |
||
| 69 | * @param VirtualRESTService|array $instance Service or info to yield the service |
||
| 70 | */ |
||
| 71 | public function mount( $prefix, $instance ) { |
||
| 72 | if ( !preg_match( self::VALID_MOUNT_REGEX, $prefix ) ) { |
||
| 73 | throw new UnexpectedValueException( "Invalid service mount point '$prefix'." ); |
||
| 74 | } elseif ( isset( $this->instances[$prefix] ) ) { |
||
| 75 | throw new UnexpectedValueException( "A service is already mounted on '$prefix'." ); |
||
| 76 | } |
||
| 77 | if ( !( $instance instanceof VirtualRESTService ) ) { |
||
| 78 | if ( !isset( $instance['class'] ) || !isset( $instance['config'] ) ) { |
||
| 79 | throw new UnexpectedValueException( "Missing 'class' or 'config' ('$prefix')." ); |
||
| 80 | } |
||
| 81 | } |
||
| 82 | $this->instances[$prefix] = $instance; |
||
| 83 | } |
||
| 84 | |||
| 85 | /** |
||
| 86 | * Unmap a prefix to service handler |
||
| 87 | * |
||
| 88 | * @param string $prefix Virtual path |
||
| 89 | */ |
||
| 90 | public function unmount( $prefix ) { |
||
| 98 | |||
| 99 | /** |
||
| 100 | * Get the prefix and service that a virtual path is serviced by |
||
| 101 | * |
||
| 102 | * @param string $path |
||
| 103 | * @return array (prefix,VirtualRESTService) or (null,null) if none found |
||
| 104 | */ |
||
| 105 | public function getMountAndService( $path ) { |
||
| 106 | $cmpFunc = function( $a, $b ) { |
||
| 107 | $al = substr_count( $a, '/' ); |
||
| 108 | $bl = substr_count( $b, '/' ); |
||
| 109 | if ( $al === $bl ) { |
||
| 110 | return 0; // should not actually happen |
||
| 111 | } |
||
| 112 | return ( $al < $bl ) ? 1 : -1; // largest prefix first |
||
| 113 | }; |
||
| 114 | |||
| 115 | $matches = []; // matching prefixes (mount points) |
||
| 116 | foreach ( $this->instances as $prefix => $unused ) { |
||
| 117 | if ( strpos( $path, $prefix ) === 0 ) { |
||
| 118 | $matches[] = $prefix; |
||
| 119 | } |
||
| 120 | } |
||
| 121 | usort( $matches, $cmpFunc ); |
||
| 122 | |||
| 123 | // Return the most specific prefix and corresponding service |
||
| 124 | return $matches |
||
| 125 | ? [ $matches[0], $this->getInstance( $matches[0] ) ] |
||
| 126 | : [ null, null ]; |
||
| 127 | } |
||
| 128 | |||
| 129 | /** |
||
| 130 | * Execute a virtual HTTP(S) request |
||
| 131 | * |
||
| 132 | * This method returns a response map of: |
||
| 133 | * - code : HTTP response code or 0 if there was a serious cURL error |
||
| 134 | * - reason : HTTP response reason (empty if there was a serious cURL error) |
||
| 135 | * - headers : <header name/value associative array> |
||
| 136 | * - body : HTTP response body or resource (if "stream" was set) |
||
| 137 | * - error : Any cURL error string |
||
| 138 | * The map also stores integer-indexed copies of these values. This lets callers do: |
||
| 139 | * @code |
||
| 140 | * list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $client->run( $req ); |
||
| 141 | * @endcode |
||
| 142 | * @param array $req Virtual HTTP request maps |
||
| 143 | * @return array Response array for request |
||
| 144 | */ |
||
| 145 | public function run( array $req ) { |
||
| 148 | |||
| 149 | /** |
||
| 150 | * Execute a set of virtual HTTP(S) requests concurrently |
||
| 151 | * |
||
| 152 | * A map of requests keys to response maps is returned. Each response map has: |
||
| 153 | * - code : HTTP response code or 0 if there was a serious cURL error |
||
| 154 | * - reason : HTTP response reason (empty if there was a serious cURL error) |
||
| 155 | * - headers : <header name/value associative array> |
||
| 156 | * - body : HTTP response body or resource (if "stream" was set) |
||
| 157 | * - error : Any cURL error string |
||
| 158 | * The map also stores integer-indexed copies of these values. This lets callers do: |
||
| 159 | * @code |
||
| 160 | * list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $responses[0]; |
||
| 161 | * @endcode |
||
| 162 | * |
||
| 163 | * @param array $reqs Map of Virtual HTTP request maps |
||
| 164 | * @return array $reqs Map of corresponding response values with the same keys/order |
||
| 165 | * @throws Exception |
||
| 166 | */ |
||
| 167 | public function runMulti( array $reqs ) { |
||
| 168 | foreach ( $reqs as $index => &$req ) { |
||
| 169 | if ( isset( $req[0] ) ) { |
||
| 170 | $req['method'] = $req[0]; // short-form |
||
| 171 | unset( $req[0] ); |
||
| 172 | } |
||
| 173 | if ( isset( $req[1] ) ) { |
||
| 174 | $req['url'] = $req[1]; // short-form |
||
| 175 | unset( $req[1] ); |
||
| 176 | } |
||
| 177 | $req['chain'] = []; // chain or list of replaced requests |
||
| 178 | } |
||
| 179 | unset( $req ); // don't assign over this by accident |
||
| 180 | |||
| 181 | $curUniqueId = 0; |
||
| 182 | $armoredIndexMap = []; // (original index => new index) |
||
| 183 | |||
| 184 | $doneReqs = []; // (index => request) |
||
| 185 | $executeReqs = []; // (index => request) |
||
| 186 | $replaceReqsByService = []; // (prefix => index => request) |
||
| 187 | $origPending = []; // (index => 1) for original requests |
||
| 188 | |||
| 189 | foreach ( $reqs as $origIndex => $req ) { |
||
| 190 | // Re-index keys to consecutive integers (they will be swapped back later) |
||
| 191 | $index = $curUniqueId++; |
||
| 192 | $armoredIndexMap[$origIndex] = $index; |
||
| 193 | $origPending[$index] = 1; |
||
| 194 | if ( preg_match( '#^(http|ftp)s?://#', $req['url'] ) ) { |
||
| 195 | // Absolute FTP/HTTP(S) URL, run it as normal |
||
| 196 | $executeReqs[$index] = $req; |
||
| 197 | } else { |
||
| 198 | // Must be a virtual HTTP URL; resolve it |
||
| 199 | list( $prefix, $service ) = $this->getMountAndService( $req['url'] ); |
||
| 200 | if ( !$service ) { |
||
| 201 | throw new UnexpectedValueException( "Path '{$req['url']}' has no service." ); |
||
| 202 | } |
||
| 203 | // Set the URL to the mount-relative portion |
||
| 204 | $req['url'] = substr( $req['url'], strlen( $prefix ) ); |
||
| 205 | $replaceReqsByService[$prefix][$index] = $req; |
||
| 206 | } |
||
| 207 | } |
||
| 208 | |||
| 209 | // Function to get IDs that won't collide with keys in $armoredIndexMap |
||
| 210 | $idFunc = function() use ( &$curUniqueId ) { |
||
| 211 | return $curUniqueId++; |
||
| 212 | }; |
||
| 213 | |||
| 214 | $rounds = 0; |
||
| 215 | do { |
||
| 216 | if ( ++$rounds > 5 ) { // sanity |
||
| 217 | throw new Exception( "Too many replacement rounds detected. Aborting." ); |
||
| 218 | } |
||
| 219 | // Track requests executed this round that have a prefix/service. |
||
| 220 | // Note that this also includes requests where 'response' was forced. |
||
| 221 | $checkReqIndexesByPrefix = []; |
||
| 222 | // Resolve the virtual URLs valid and qualified HTTP(S) URLs |
||
| 223 | // and add any required authentication headers for the backend. |
||
| 224 | // Services can also replace requests with new ones, either to |
||
| 225 | // defer the original or to set a proxy response to the original. |
||
| 226 | $newReplaceReqsByService = []; |
||
| 227 | foreach ( $replaceReqsByService as $prefix => $servReqs ) { |
||
| 228 | $service = $this->getInstance( $prefix ); |
||
| 229 | foreach ( $service->onRequests( $servReqs, $idFunc ) as $index => $req ) { |
||
| 230 | // Services use unique IDs for replacement requests |
||
| 231 | View Code Duplication | if ( isset( $servReqs[$index] ) || isset( $origPending[$index] ) ) { |
|
|
|
|||
| 232 | // A current or original request which was not modified |
||
| 233 | } else { |
||
| 234 | // Replacement request that will convert to original requests |
||
| 235 | $newReplaceReqsByService[$prefix][$index] = $req; |
||
| 236 | } |
||
| 237 | View Code Duplication | if ( isset( $req['response'] ) ) { |
|
| 238 | // Replacement requests with pre-set responses should not execute |
||
| 239 | unset( $executeReqs[$index] ); |
||
| 240 | unset( $origPending[$index] ); |
||
| 241 | $doneReqs[$index] = $req; |
||
| 242 | } else { |
||
| 243 | // Original or mangled request included |
||
| 244 | $executeReqs[$index] = $req; |
||
| 245 | } |
||
| 246 | $checkReqIndexesByPrefix[$prefix][$index] = 1; |
||
| 247 | } |
||
| 248 | } |
||
| 249 | // Run the actual work HTTP requests |
||
| 250 | foreach ( $this->http->runMulti( $executeReqs ) as $index => $ranReq ) { |
||
| 251 | $doneReqs[$index] = $ranReq; |
||
| 252 | unset( $origPending[$index] ); |
||
| 253 | } |
||
| 254 | $executeReqs = []; |
||
| 255 | // Services can also replace requests with new ones, either to |
||
| 256 | // defer the original or to set a proxy response to the original. |
||
| 257 | // Any replacement requests executed above will need to be replaced |
||
| 258 | // with new requests (eventually the original). The responses can be |
||
| 259 | // forced by setting 'response' rather than actually be sent over the wire. |
||
| 260 | $newReplaceReqsByService = []; |
||
| 261 | foreach ( $checkReqIndexesByPrefix as $prefix => $servReqIndexes ) { |
||
| 262 | $service = $this->getInstance( $prefix ); |
||
| 263 | // $doneReqs actually has the requests (with 'response' set) |
||
| 264 | $servReqs = array_intersect_key( $doneReqs, $servReqIndexes ); |
||
| 265 | foreach ( $service->onResponses( $servReqs, $idFunc ) as $index => $req ) { |
||
| 266 | // Services use unique IDs for replacement requests |
||
| 267 | View Code Duplication | if ( isset( $servReqs[$index] ) || isset( $origPending[$index] ) ) { |
|
| 268 | // A current or original request which was not modified |
||
| 269 | } else { |
||
| 270 | // Replacement requests with pre-set responses should not execute |
||
| 271 | $newReplaceReqsByService[$prefix][$index] = $req; |
||
| 272 | } |
||
| 273 | View Code Duplication | if ( isset( $req['response'] ) ) { |
|
| 274 | // Replacement requests with pre-set responses should not execute |
||
| 275 | unset( $origPending[$index] ); |
||
| 276 | $doneReqs[$index] = $req; |
||
| 277 | } else { |
||
| 278 | // Update the request in case it was mangled |
||
| 279 | $executeReqs[$index] = $req; |
||
| 280 | } |
||
| 281 | } |
||
| 282 | } |
||
| 283 | // Update index of requests to inspect for replacement |
||
| 284 | $replaceReqsByService = $newReplaceReqsByService; |
||
| 285 | } while ( count( $origPending ) ); |
||
| 286 | |||
| 287 | $responses = []; |
||
| 288 | // Update $reqs to include 'response' and normalized request 'headers'. |
||
| 289 | // This maintains the original order of $reqs. |
||
| 290 | foreach ( $reqs as $origIndex => $req ) { |
||
| 291 | $index = $armoredIndexMap[$origIndex]; |
||
| 292 | if ( !isset( $doneReqs[$index] ) ) { |
||
| 293 | throw new UnexpectedValueException( "Response for request '$index' is NULL." ); |
||
| 294 | } |
||
| 295 | $responses[$origIndex] = $doneReqs[$index]['response']; |
||
| 296 | } |
||
| 297 | |||
| 298 | return $responses; |
||
| 299 | } |
||
| 300 | |||
| 301 | /** |
||
| 302 | * @param string $prefix |
||
| 303 | * @return VirtualRESTService |
||
| 304 | */ |
||
| 305 | private function getInstance( $prefix ) { |
||
| 322 | } |
||
| 323 |
This check looks for the bodies of
ifstatements that have no statements or where all statements have been commented out. This may be the result of changes for debugging or the code may simply be obsolete.These
ifbodies can be removed. If you have an empty if but statements in theelsebranch, consider inverting the condition.could be turned into
This is much more concise to read.