Build

It matters that a project builds and runs. Sometimes projects get submitted for review that don't meet even that bar.

Of course, my project didn't run, because a lot has changed since then:

• Had to bump AGP from 1.5.0 to 8.1.2
• Declare the namespace in build.gradle instead of AndroidManifest.xml
• Set android:exported on every Activity in the manifest
• Switch dependencies from compile to implementation
• Use google() and mavenCentral() instead of jcenter()
• Set android.nonFinalResIds=false in gradle.properties
• Update gradle-wrapper.jar
• Bump Gradle from 2.8 to 8.2.1 in gradle-wrapper.properties
• Update gradlew and gradlew.bat
• Replace the reference to R.attr.colorPrimaryDark with android.R.attr.colorPrimaryDark

If someone sent me a project like this today, I'd flag it as a serious minus — a reviewer shouldn't have to make the project work themselves. But since this project was made a long time ago, I'll give the author a pass here. And at the time, it did all work.

Total: 1 point for the fact that it built and ran back in its day.

Repository Setup

This covers the following criteria:

• Task breakdown in the README
• Task estimation
• A clear commit history

I have none of this: I never wrote a breakdown back then, never estimated tasks, and didn't use Git at all. So the project gets zero points for this section.

Total: 1 point + 0 = 1 point.

Libraries

What I'd want to see in a project like this today:

• Kotlin
• Kotlin Coroutines
• Jetpack Compose
• OkHttp
• Retrofit / Ktor
• Gson / Moshi / Kotlinx Serialization

Kotlin

My project uses Java. If I'd made it a year later, that would be fair to criticize, but Google only announced official Kotlin support on Android in 2017.

Kotlin Coroutines

Stable Kotlin Coroutines showed up much later, so there's no way I could have used them. For async work I used AsyncTasks. At the time that was probably more or less fine, though my very first project at the company already used RxJava, so it'd be fair to expect the same here.

Jetpack Compose

Jetpack Compose obviously didn't exist yet either, so layout was simpler — there wasn't much to choose from in the first place.

OkHttp

This library definitely existed at the time, so I'd expect the project to use it both now and back then. And, to my surprise, I did use it! So that one's a pass.

Retrofit / Ktor

I couldn't have been expected to use Ktor back then, but Retrofit was also googleable and could've been dropped in without much trouble. So I could've earned a point here too, but no such luck.

Gson / Moshi / Kotlinx Serialization

Both Gson and Moshi already existed at the time — Moshi's first version came out in 2015, and Gson all the way back in 2008 — so it would've been reasonable to use one of them to handle JSON and keep the code cleaner and easier to follow. But I parsed and built everything by hand, no libraries. Sad, but that's how it is.

Total: 1 point + 1 point for OkHttp = 2 points. In my case I could've gotten: 1 point for RxJava, 1 point for a serializer, 1 point for Retrofit.

Architecture

What I'd want to see in a project like this today:

• MVI / MVVM
• Clean Architecture
• DI

MVI / MVVM

For the UI layer pattern I used MVC. You could say there was no pattern at all. I think even MVP could've been implemented at the time without much trouble, so this is a clear shortfall.

Clean Architecture

I hadn't even heard of Clean Architecture back then, but somehow managed to distribute classes reasonably sensibly by purpose: widgets, net, ui, model, json.

It's not actually that bad — the code isn't dumped into one pile, and it's generally clear where things live. I think expecting full Clean Architecture understanding from an intern is excessive, so I'll give this part a pass.

But what could've been improved:

• Split by feature: posts, collage, share
• Merge json and net together
• Move entities out of model into entity, and rename model to domain
• For some reason all the logic classes are named Factory — at minimum I'd rename them to Interactor

DI

In principle, any solution that let you provide dependencies into classes from the outside would've worked here. Some things in the project are passed in that way, but not all — some classes are created inside methods, which isn't great. For example, in the InstagramCollageFactory class, in the generateCombinations method:

private void generateCombinations(int size) {  
    PermutationsGenerator permutationsGenerator = new PermutationsGenerator();  
    sInstagramImgsCombinations = permutationsGenerator.getCombinations(size);  
}

I'd have liked to see more attention paid to this in the project, so this is another shortfall.

Total: 2 points + 1 point for reasonable class distribution = 3 points.

Code Quality

No Hardcoded Strings

All the strings used in the UI are extracted into strings.xml, and there are even multiple localizations, even though the assignment didn't require it. That said, there are a few places with hardcoded strings, but they're log messages, so I'll let that slide.

Code Split Into Components

The main things worth praising here:

• As already mentioned, there's a split into classes by package, which helps you navigate the code better.
• Each component has a clear purpose and contains exactly what you'd expect to find in it. For example: screens are split into fragments; there are classes responsible for posts, the collage, the user; there are separate classes for networking, and so on. Overall, I never got the sense that anything was missing or redundant.
• There are some base entities in place, so there's no duplicated code.

What could be improved:

• Maybe I shouldn't have used Activities that just display a single fragment and contain no logic of their own — navigation could've been handled purely through fragments instead.
• Putting caching logic inside a data class (InstagramPost) also looks wrong — there's a clear mixing of concerns, and the class ended up doing more than it should.

Code Tidiness

Magic Numbers

There are places with magic numbers, for example in the onCreateView method of CollageFragment:

public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {  
	   ...
	   
       mEmptyLinearLayout = (LinearLayout) v.findViewById(android.R.id.empty);  
       mEmptyLinearLayout.setVisibility(View.GONE);  
       setCollageButton(4);  
  
       return v;  
    }

And the method name doesn't help you figure out what the number being passed means. If it were obvious, leaving it as-is would be fine, but here it's a real problem.

Unused Variables

There are unused variables, for example in the CollageFragment class:

private static final String KEY_TASK_STATUS = "taskStatus";  
private static final String KEY_PROGRESS_DIALOG = "mProgressDialog";  
private static final String TAG = "CollageFragment";

And it's not an isolated case — there are plenty of places where Context is passed in but never used, which is a more serious problem.

Commented-Out Code

There are quite a few places with commented-out chunks of code. One example, from CollageFragment:

    @Override  
    public void onSaveInstanceState(Bundle outState) {  
       super.onSaveInstanceState(outState);  
       outState.putInt(KEY_POSTS_COUNTER, checkedPostsCounter);  
//     outState.putBoolean(KEY_TASK_STATUS, isGetPostsTaskRunning);  
    }

Using Git would've made keeping these dead bits around unnecessary. It might look minor, but it makes the code noticeably harder to read.

Useless Comments

Oddly enough, there's almost nothing to complain about here: I hardly wrote any of the "increment the counter by one" style explanatory comments. The one thing that fits this bucket is a bunch of commented-out Log calls scattered through almost every method. For example, in the positionsOnlyCheckedItems method of CollageFragment:

//		Log.d(TAG, "Positions of checked posts (before): " + checkedItemPositions.toString());  
//		Log.d(TAG, "Size: " + checkedItemPositions.size());  
		...
//		Log.d(TAG, "Positions of checked posts (after): " + checkedItemPositions.toString());

This overlaps a bit with the previous point, but while commented-out logic can still be understood somehow, commented-out logs are pure noise that should've been deleted right away.

Variable Naming

Variable naming tries to follow Hungarian notation, but there are exceptions that can't be explained by anything other than carelessness or being in a rush. For example, in the CollageFragment class you can see some variables starting with m while others don't, even though they're at the same level:

private ImageButton mBackImageButton;  
private TextView mSelectedPostsCounterEditText;  
private Button mCollageButton;  
private int checkedPostsCounter = 0;  
private PostAdapter adapter;

Total: 3 points + 2 points for extracting strings and a sensible split into components = 5 points. Code tidiness (magic numbers, unused variables, commented-out chunks, and inconsistent notation) cost me a couple more points.

Design

What I'd want to see in a project like this today:

• Match with the mockup
• Adaptive layout
• Skeletons during loading
• Placeholders for content being loaded
• Use of styles and a theme
• Support for theme switching

Match With the Design

I found the mockup and, surprisingly, implemented it pretty closely: its own color palette (colors.xml), custom Roboto fonts, cards (CardView) in the post list, tidy toolbars. It's all put together carefully and looks like what was intended. For an intern project, this even exceeds expectations, so I'll give it a point.

Adaptive Layout

This one genuinely surprised me: the project has separate layouts for landscape orientation and different API versions — layout-land, layout-land-v11, layout-v11 — plus all dimensions are extracted into dimens.xml. So I did think about rotations and different screens back then. A point.

Skeletons

There are no skeletons. But, same as with Compose, this wasn't a common practice at the time, so I won't hold my past self to that standard.

Loading Placeholders

This one's a mixed bag. There are no placeholders while images are loading — the ImageView just stays empty while the bitmap loads. But for empty list states there are placeholders, covering three cases at once: a locked account, no network, and no posts — each with its own icon (locked_media_icon, internet_off_icon, without_media_icon) and text. I won't award a point for loading placeholders, but I mentally credit the thoughtful empty states.

Use of Theme Styles

Everything's good here: there's a full AppTheme with colorPrimary/colorPrimaryDark/colorAccent, styles for buttons, the toolbar, and lists in styles.xml, and even a custom fontCustom attribute via attrs.xml. No hardcoded colors or dimensions in the layout — everything goes through resources and the theme. A point.

Theme Switching Support

Themes can't be switched — DayNight didn't exist yet, there's only one theme. So, like with skeletons, this doesn't count against me.

Total: 5 points + 3 points for matching the mockup, adaptivity, and working with styles/theme = 8 points.

Logic

What I'd want to see in a project like this today:

• Error handling
• Pull-to-refresh
• A navigation abstraction
• Correct behavior on rotation
• The app launches and doesn't crash
• No leaks
• Proper image loading
• Logical separation of responsibilities

Error Handling

There's basic handling: before a request, network availability is checked (NetworkConnector.isConnection) and a Toast is shown, and empty states have dedicated screens prepared. But in the network layer itself, errors are simply swallowed — printStackTrace() and return null:

} catch (IOException ex) {  
    ex.printStackTrace();  
}  
return stringResponce;

So the actual error never reaches the user, and null propagates further through the code instead. Fine for an intern, but ideally the handling should've been taken further.

Pull-to-Refresh

There's no pull-to-refresh. Refreshing is done via a toolbar button (menu_item_refresh_posts) rather than the familiar SwipeRefreshLayout gesture. Not critical, but no point for this one.

Navigation Abstraction

There's no abstraction over navigation at all. Transitions happen directly in click handlers via startActivity:

Intent intent = new Intent(getActivity(), PublishActivity.class);  
intent.putExtra(PublishFragment.EXTRA_IMAGES_IDS, checkedPostsIDs);  
startActivity(intent);

On top of that, every screen has its own Activity (CollageActivity, PublishActivity, ShareActivity) sitting on top of SingleFragmentActivity. This echoes what I already noted about redundant Activities. No point.

Rotations Work Correctly

Here I did take care of things: the fragment has setRetainInstance(true) set, the counter value is saved in onSaveInstanceState, and there are landscape layouts. So on rotation the list isn't lost and loading doesn't restart. A point.

The App Launches

It launches — but, as we already established in the Build section, only after I nursed the project back to life. Back in the day it did work, and that point was already counted there, so I won't award it again.

The App Doesn't Crash

This is where the real problems are. The scariest spot is image loading: NetworkConnector.getBitmapFromURL kicks off an AsyncTask and then immediately blocks waiting for the result via .get():

public Bitmap getBitmapFromURL(String url) {  
    bitmapURL = url;  
    AsyncTask<Void, Void, Bitmap> requestBitmap = new GetBitmap().execute();  
    Bitmap postImage = null;  
    try {  
        postImage = requestBitmap.get();  
    } ...  
}

And this is called from the adapter's getView, meaning the network is effectively hit on the main thread — a direct path to freezes and ANRs. On top of that, mDirectory.list() can throw an NPE in places if the folder doesn't exist yet. So I can't in good conscience mark "doesn't crash" here.

No Leaks

There's plenty to pick at here too. The AsyncTask holds a reference to the fragment listener (though setRetainInstance(true) partially saves it), and there's a lingering ProgressDialog. The cache is also cleared in a rather hacky way — via a shell command:

String deleteCmd = "rm -r " + mDirectory;  
Runtime.getRuntime().exec(deleteCmd);

It's unsafe, async, and swallows errors. No point.

Image Loading

I load and cache images by hand: BitmapFactory.decodeStream over the network plus my own file cache in BitmapWorker (with a CACHE_MAXINUM = 20 limit). No inSampleSize, meaning bitmaps get decoded at full size — hello OutOfMemory. And all of this happens on the main thread (see above). Both Glide and Picasso already existed in 2015 — I should've grabbed either one instead of reinventing the wheel. A minus.

Logical Separation of Responsibilities

The UI controller decides where data comes from — cache or a request — even though that's not its job. For example, in the onCreate method of CollageFragment:

if ((mGotIstagramId != null) && !isGetPostsTaskRunning) {  
    if (!InstagramPostsFactory.getFactory(getContext()).getInstagramPostsStatus(mGotIstagramId)) {  
       BitmapWorker.deleteAllBitmapsFromCacheDirectory(getContext());  
       mGetPostsTask = new GetPostsTask(this);  
       mGetPostsTask.execute(mGotIstagramId);  
    } else {  
       mPosts = InstagramPostsFactory.getFactory(getContext()).returnInstagramPosts();  
    }  
}

Not only does the fragment decide where data comes from, it also clears the cache itself and kicks off the task itself. And the logic for loading and caching images lives entirely inside the InstagramPost data class (the getPostsImageWithCache method), which I already flagged as a mixing of responsibilities. Overall, the responsibilities are scattered in the wrong places. No point.

Total: 8 points + 2 points for correct rotation handling and basic error handling = 10 points. Here I could've picked up more points for loading images through a library, avoiding main-thread work, and a sane separation of responsibilities.

What Stood Out During a Closer Review

Checklist aside, while digging through the code I found a few spots worth calling out separately — they show my level at the time particularly well, and also explain the scores above.

Homemade Multithreading With a Busy-Wait

The most telling find is combination generation for the collage. In PermutationsGenerator I kick off a background thread through a homemade "Executor", then wait for it in an infinite loop, polling the result size and sleeping for a second:

public HashSet<ArrayList<String>> getCombinations(int size) {  
    ...  
    GenerateExecutor mExecutor = new GenerateExecutor();  
    mExecutor.execute(new RunnableForGenerateExecutor());  
  
    while (true) {  
        if (mCombinations.size() >= 1) {  
            try {  
                Thread.sleep(1000);  
            } catch (InterruptedException e) { ... }  
            return mCombinations;  
        }  
    }  
}

There's a whole bouquet of problems here at once: a busy-wait spinning the CPU for nothing; a race on a shared HashSet with zero synchronization; and a magic Thread.sleep(1000) hoping the thread will definitely finish counting within a second. It apparently worked on a wing and a prayer. Today this wouldn't survive even the most superficial review.

Global Singletons Holding a Context

Almost my entire "model" layer is static singletons that also hold onto a Context:

public static InstagramPostsFactory getFactory(Context c) {  
    if (sInstagramPostsFactory == null) {  
        sInstagramPostsFactory = new InstagramPostsFactory(c);  
    }  
    return sInstagramPostsFactory;  
}

Right alongside it lives mutable static state (sUserID, sInstagramImgsCombinations). That's both a potential leak (a Context held statically) and global state that can't be properly tested or reasoned about. Exactly the kind of case where a missing DI setup hits everything at once.

A Flag-Based Cache State Machine

I determine whether the post cache is valid with this tangle of conditions in getInstagramPostsStatus:

if (((sUserID == null) && (mInstagramPosts == null)) || ((sUserID != null) && !sUserID.equals(id))) {  
    sUserID = id;  
    status = false;  
} else if (... mInstagramPosts.size() == 0 ...) {  
    ...  
} else if ((sUserID != null) && (mInstagramPosts != null) && sUserID.equals(id)) {  
    status = true;  
}

Understanding what's going on here on the first read is nearly impossible — and all it answers is "is there a valid cache for this user?". Today that's a couple of lines through a repository with clear state on a StateFlow.

These findings don't change the score — they just explain why I'm not willing to give myself more for architecture and logic.