This function is doing WAY too much.
- you are storing unnecessarily hard-coded default configuration
- storing OAuth configuration (where it is not clear how you handle OAuth for individual end users)
- implementating JSON caching mechanism
- interacting with Twitter API
- making (seemingly) trivial transformation on the data returned from API
You should STRONGLY consider taking an object-oriented approach here. I think that, because your function is doing too much, you have great opportunity to refactor this code into appropriate classes/objects which each handle a slice of the overall behavior.
You should also strongly consider not hard-coding values that should probably be configurable by the caller. Why would you build a function that could only get tweets for a single user name? Why would you not pass in values such as username, number of tweets to retrieve, where the caching directory should lie, etc.?
You need to think about interacting with the caller in a consistent way. Let's consider this code:
$some_result = get_twitter_json();
Right now when I call
get_twitter_json(), several things can happen:
- You will output to standard out around directory existence/non-existence. Why is this function outputting directly at all? There is nothing in function name that would suggest to caller that it could generate output. If you have a failure scenario, you should probably log an error and then
return a meaningful value to caller to indicate failure or throw an exception depending on how you want to the handle the error.
- You potentially echo a very generic and probably useless error message around a problem making the request. You should log errors, not output them. Let the caller decide how to handle the messaging to this user, not this function. Again, why would this function possibly produce spurious output to standard out? You are working with a known API, this code should likely handle ALL known error codes/messages that this API generates and perform appropriate error logging or throwing of exception depending on how you are expecting the caller to handle the various flavors of error (i.e. are they recoverable?).
- If the operation is successful, either via cache retrieval or API call, the function just silently completes, nothing is returned to the caller to indicate success.
- So this brings me back to that example code. When caller is invoking this function, what would you expect to be returned to populate
$some_result? Right now, absolutely nothing is returned, no errors logged, no exceptions thrown to indicate to the caller whether the intended action was successful or not. When contrasting this behavior with the function name of
get_*, I cannot understand as a caller what I should expect to happen. Typically methods prefixed with
get actually return something or a value like
null when the operation fails. I think you have a poorly named function that doesn't express what is actually happening maybe something like
persist_json_from_twitter_feed() would be more meaningful.
I honestly don't understand the branch of logic here which reads from cached representation. Why bother even doing that if you aren't actually returning the JSON? Even the branch where you interact with the Twitter API does not return anything to the caller (see earlier comment), but does write to the cache. Is this really just a function to update the cache from the twitter feed? If so, why so you ever need to read the existing cache file and
Stylistically, you have some problems. I think the answer given by @Sumurai8 addresses several of these. I would add that you have no meaningful comments. Oftentimes, well-written code with meaningful variable, class, function, etc. names only needs minimal commenting around logical considerations. Here you lack both comments and well-chosen function/variables names.
I am guessing that this bit of code (which is inserted inside a
json_decode() call) likely has potential need for you to provide some error handling. Does this class throw exceptions such that this code should be within try-catch block? What happens if Oauth fails? What happens if request itself fails?
Your code really only covers "happy path" use cases and is likely to be significantly fragile.