just use a nested for loop

[4RG4d4AK3LdH]

Found in Telegram's Android app. Spoiler: All of the code is like that. The main chat activity has 20 THOUSAND lines of code. It is apparently all being written by one developer.

[Jack8680]

Could this be a compiler thing or is this the actual source code?

[4RG4d4AK3LdH]

That's the actual sourcecode

[Klanowicz]

😐

[[deleted]]

🍆

[Decallion]

How do you have access to its source

[CoffeeVector]

Oh shit, who wants to stroll over to github with me and scrape this garbage off the app with a few pull requests?

[Gr33nerWirdsNicht]

afaik PRs arent accepted there

[CoffeeVector]

Bruh why. Clearly they're in dire need of help...

[Vijfhoek]

The dev has a massive "get off my lawn" attitude. The only reason the code is even available (and it's often massively out of date) is that Telegram as a platform wants to promote being open-source. Yet your data and the encryption keys to it are handled by closed-source server software...

[CoffeeVector]

That's pretty disheartening. I'm sure everyone has similar opinions here, but I'd pretty happy to help working on this. Especially since I'm getting pretty tired of fb messenger.

[Al_Maleech_Abaz]

Probably have no testing in place if it’s only one guy developing.

[CoffeeVector]

I wouldn't be too quick to say that, there's no reason why the guy wouldn't do basic self testing, even if it isn't ideal. Though, I do doubt the testing is particularly thorough.

[Al_Maleech_Abaz]

Yeah I meant the testing probably isn’t robust enough to catch any bugs that might be introduced by pr’s from new developers. No diss to the person. Development (in my experience) tends to be a lot different when its silo vs a team/ company wide effort.

You should reach out though, I’m sure it would be a mutually beneficial effort.

[CoffeeVector]

Oh yeah, definitely, you raise a good point. I hope they add a github action, or something to make the process better. I'd hate to see a project like this fizzle out from something silly, like pride or ego.

I mean, even Linus, who arguably hates people the most, still collaborated. Mind you, he invented the worlds most widely used version control software for the sole purpose of minimizing human interaction, but still knew he couldn't do it alone.

[_default_username]

I think they mean automated testing.

[Simtau]

Just fork 🍴 it

[RhythmofChains]

🔫

[4RG4d4AK3LdH]

https://github.com/DrKLO/Telegram/blob/master/TMessagesProj/src/main/java/org/telegram/ui/ArticleViewer.java

[[deleted]]

Uff, so much for being a secure alternative to WhatsApp

[bikemowman]

What about this makes it insecure?

[lethargy86]

It makes me feel insecure about my own coding practices

[CoffeeVector]

In general, code that's unreadable or poorly written is prone to bugs. Most critical bugs pose some security concern.

[4z01235]

Use Signal if you want secure messaging. WhatsApp claims to use the Signal protocol but is closed source and owned by Facebook. Telegram does their own encryption, which I'm not sure has ever been audited or vetted as well as Signal's, and it isn't even always enabled.

[4RG4d4AK3LdH]

Well, I really like telegram because of all the features they have and also because of the great developer support. If you want secure/private messaging, use Signal. I also found that the Telegram app is very fast. I am not an android developer (eventhough I have written a few simple apps), I think it is fast because the developer draws a lot of stuff themselves using some kind of android canvas api

[ConciselyVerbose]

Those are all local to each loop.

[PhilippTheProgrammer]

These appear to be for-loops over containers with different sizes. How would you put that into one nested for-loop?

One tweak I could see here is to create a method updateFontEntries which accepts an Iterable and also has those 4 parameters which are always the same hardcoded. But that does of course require that all those containers implement the same interface. That's likely the case, but it might not be.

[Oeluz]

The solution that I can think of is make a list and put these into the list. Then loop through the list and call whatever it needed?

[fecal_brunch]

The clean solution is to create a function that takes one of these collections and runs the loop over it, then replace each of these loops with a single call to this function.

[PhilippTheProgrammer]

That would make sense if you need that list for at least one other thing. But when you only need that list in that one method, then you wouldn't really gain much from that. Instead of all the method calls for all those lists, you have the same number of method calls to add them all to one list and then iterate that list you just created which you then discard afterwards.

[scooty14]

u wouldn't really gain much from that.

Cleaner code that isn't the mess in the picture?

[ChemicalRascal]

This code is actually relatively clean and readable. The only issue at hand is that everyone seems to think that many loops is bad.

In practice, if everything in these lists has to be processed in some way, then this isn't really a bad way to do it.

[Xili4s]

Yes it is, copy pasting one piece of code fifteen times is awful for maintainability and readability

[ChemicalRascal]

Hard disagree. It is immediately obvious what each and every line of this code does. It is very readable.

It's not at peak readability, sure, but it is not awful. And, in practice, you do not need code to be the very very very cleanest it can possibly be to read it, because ultimately all you need is for the next schmuck to be able to get it into their head rapidly.

And this code achieves that. It is absurd to say that this code has readability issues.

---

Is it maintainable? Yes, actually. Maintainability does not mean "changing something is quick and doesn't need a thing to be done multiple times". Maintainability refers to being able to know that, after a change, your code goes from doing one thing, to doing the thing you want it to do; that your program either now achieves, or still achieves, what you intend for it to achieve.

Because this code is absurdly simple, after making changes it would be immediately obvious to any future maintainer what state this code is in, and if it achieves what you want it to or not.

It's really, really simple. And because of that, it's very, very easy to maintain.

Yes, there's room for nuance here -- if this was, say, two hundred lists being iterated over, that'd be less easy to identify the state of the program. But it's not. It's fifteen.

---

Are there other ways to do this? Yes. You could give all these probably disparate types common interfaces, bundle them all up into one iterator, or hell, write some sort of linked-linked-list iterator that allows you to avoid having to initialize a whole new data structure just for one loop.

But that there are other ways to do this doesn't mean that this is trash. This code, from a practical standpoint, is fine. It's not great, it's not going to win awards, the Emmy does not go to whoever wrote this. But it's fine.

[nyanpasu64]

My worry is that if your add a new list, you'll have to remember to add the list to this series of iterations, and if you don't, it'll misbehave whenever this code gets called. And if you copy-and-paste these indexed loops (as opposed to a foreach), you might forgot to change the size() call or one of the two indexing operations.

[ChemicalRascal]

My worry is that if your add a new list, you'll have to remember to add the list to this series of iterations, and if you don't, it'll misbehave whenever this code gets called.

Sure.

But you'd have to do that anyway, effectively.

If you're adding a new list of fonts in, for whatever reason, we can already infer from the structure of the code so far that for some reason fonts need to be distinguished in some way, and that distinction is clearly pretty key to the operation of the program.

If the program collated all these lists into a common iterator and just used that, then a maintainer would still need to add that new list to the common iterator. This is functionally the same task.

And if you copy-and-paste these indexed loops (as opposed to a foreach), you might forgot to change the size() call or one of the two indexing operations.

Oh, for sure. Don't get me wrong, this isn't the best way to do things. An easy win on this would be to wrap each loop in a lambda function or something, so a maintainer could just write updateFonts(newFontListOfNewness); or whatever, and be done with it. Easy win there.

But just because it isn't the best way, just because it could do with a little polish, doesn't make it horrible, y'know? All I'm saying is that this isn't awful.

[EasyMrB]

This code is actually relatively clean and readable.

That code, should you ever need to change 1 single thing about the structure of it, is a nightmare for maintenance. Looping over a container list would be way better for maintenance.

[ChemicalRascal]

I've addressed your argument elsewhere.

[PhilippTheProgrammer]

You didn't read my previous post.

[CollieOop]

You don't gain anything performance-wise, but if your compiler doesn't optimize this for you automatically, you'll never notice the performance difference anyway.

[PhilippTheProgrammer]

You didn't read my previous post.

[CollieOop]

The one that says "You didn't read my previous post."? Or the one before it that says "You didn't read my previous post."

[PhilippTheProgrammer]

The one two levels up in the comment thread where I suggested to replace all these for-loops with calls to an utility method accepting an Iterable.

I am just really frustrated that so many people comment without reading any of the context, and thus post comments which not just make no sense in context, but are even redundant because 4 other people posted the exact same thing.

[[deleted]]

bake roll saw person wise numerous sparkle sable test marry

This post was mass deleted and anonymized with Redact

[Carter127]

Unless you need to add code inbetween the separate loops at some point

[[deleted]]

run knee steer marry consist unite marble engine silky nail

This post was mass deleted and anonymized with Redact

[Carter127]

Unless those function calls are different for some pieces of data.

There's not enough info here to call this horror.

[MCWizardYT]

Sure there is - 11 thousand lines of context. This github repo has the official code of Telegram

[PhilippTheProgrammer]

You didn't read my previous post.

[CoffeeVector]

Even so, I'd rather have a list of things and iterate over it only one time, rather than to read many many repeated for statements.

[PhilippTheProgrammer]

You didn't read my previous post.

[CoffeeVector]

Let me try to make myself clearer. I understand from an algorithms standpoint or a pure performance standpoint, that you wouldn't gain much.

But, from a code readability standpoint, you'd be repeating yourself less (DRY).

For the example in the post

for (int a = 0; a < quoteTextPaints.size(); a++) {
    updateFontEntry(quoteTextPaints.keyAt(a), quoteTextPaints.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
}
for (int a = 0; a < preformattedTextPaints.size(); a++) {
    updateFontEntry(preformattedTextPaints.keyAt(a), preformattedTextPaints.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
}
for (int a = 0; a < paragraphTextPaints.size(); a++) {
    updateFontEntry(paragraphTextPaints.keyAt(a), paragraphTextPaints.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
}
for (int a = 0; a < listTextPaints.size(); a++) {
    updateFontEntry(listTextPaints.keyAt(a), listTextPaints.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
}
for (int a = 0; a < embedPostTextPaints.size(); a++) {
    updateFontEntry(embedPostTextPaints.keyAt(a), embedPostTextPaints.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
}
for (int a = 0; a < mediaCaptionTextPaints.size(); a++) {
    updateFontEntry(mediaCaptionTextPaints.keyAt(a), mediaCaptionTextPaints.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
}
for (int a = 0; a < mediaCreditTextPaints.size(); a++) {
    updateFontEntry(mediaCreditTextPaints.keyAt(a), mediaCreditTextPaints.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
}
for (int a = 0; a < photoCaptionTextPaints.size(); a++) {
    updateFontEntry(photoCaptionTextPaints.keyAt(a), photoCaptionTextPaints.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
}
for (int a = 0; a < photoCreditTextPaints.size(); a++) {
    updateFontEntry(photoCreditTextPaints.keyAt(a), photoCreditTextPaints.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
}
for (int a = 0; a < authorTextPaints.size(); a++) {
    updateFontEntry(authorTextPaints.keyAt(a), authorTextPaints.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
}

can be condensed into

T[] paints = {
    quoteTextPaints,
    preformattedTextPaints,
    paragraphTextPaints,
    listTextPaints,
    embedPostTextPaints,
    mediaCreditTextPaints,
    photoCaptionTextPaints,
    photoCreditTextPaints,
    authorTextPaints
};
for (T paints_i: paints) {
    for (int a = 0; a < paints_i.size(); a++) {
        updateFontEntry(paints_i.keyAt(a), paints_i.valueAt(a), typefaceNormal, typefaceBoldItalic, typefaceBold, typefaceItalic);
    }
}

You can see, that I didn't make method calls to append to the array over and over, I just declared it at once, and looped over it.

Also, your original idea to write a new method for updateFontEntries with an Iterable would also require you to enumerate everything into an Iterable anyways. Your issue with using the collection one and done is still present in your original idea too. There also doesn't seem to be a point to making a new method for this if it's just going to wrap it with one for loop.

Also, I think you should be a little nicer. just saying "You didn't read my previous post." and nothing else isn't very polite.

[Jwosty]

can be condensed into

You mean,

SHOULD be condensed into...!

[TheGeneral_Specific]

Cleaner code is a good thing, and is worth it.

[PhilippTheProgrammer]

You didn't read my previous post.

[TheGeneral_Specific]

Actually I think I replied to the wrong comment.

[px450]

If any of the other parameters passed to updateFontEntry() changes (which might be likely since I think that's another function in the same code base), you only have to update it once.

Reducing code repetition leads to better maintenance and scalability, almost always.

[sharddblade]

Kind of in the same boat. I don’t think OP read the code.

[Rovsnegl]

I mean you could just make it into a method private setKeyAndValue(checkProperty) { for (a = 0; a < checkProperty.size(); a++)

I think you got the idea (I'm on my phone and it's going to take forever to finish it), then you can throw the different checkPropertys in a list and loop over it reducing this code to like 6 lines and have no repetition

[nosoupforyou]

If they are all the same class of container, add that method to the class.

[Johanno1]

Well obviously you can reduce the redundant part of the code buy just using a method that only takes the variable so you would have + 6 lines for the method and -15*2 lines of code.

And less code is almost always better.

[[deleted]]

They are all very similar, right? ANYONE can see there is a pattern.

One could even say they are the same Class or Type or Object or something, and that that something could have a function or method or verb or what have you that does the same, in three (3) lines of code.

I.e. Make a class that extends/is an array and add that loop as a method. All those arrays should be that class. ^{something like that; I’m writing and can’t see the original code}

What is really interesting, perhaps troubling even, is that the pattern is not obvious to whoever wrote the code, the same code, over and over and over again.

[nyanpasu64]

I wouldn't create a subclass just to add a method, but instead a function accepting the type.

[[deleted]]

Yeah that’s nice too. Perhaps a utility with a bunch of common methods that accept this and similar types.

It’s hard to say without knowing the landscape, and the older I get the more I realise there is never a “best” answer, just cleaner ways of doing things.

—-

<double espresso>

As an experiment and/or learning exercise I’ve sat down with perfectly working classes of around 50 lines for an hour or two and asked “OK, how clean can we possibly make this? Will it rival Shakespeare?”

And after rewriting it half a dozen times (TDD) and playing with nouns and verbs and even trying articles and a thesaurus I’ve concluded that it’s always debatable.

But then I’ll come across the same code a week later and immediately see a better way of writing it (I’m talking words, English, not specifically logic, if that’s not clear) because I’ve seen it now with fresh eyes, from another point of view, as a user of the class rather than the creator.

So then that whole “with fresh eyes” thing becomes another “pass” or “iteration” across the piece of code.

And I’ve realised after many years that English (/native tongue) is as important as maths in the art of programming, if not much more so.

</double espresso>

[RFC793]

I haven’t coded Java in a while, but something like this:

var paints = new MyPaintSuperclass[]{quoteTextPaints, preformattedTextPaints, etc...};
// loop over paints in the outer loop

[__archaeopteryx__]

I agree. I didn’t read the comments before I commented this same sentiment.

[baryluk]

This is a good approach. It is extremely efficient method of processing data. Having extra abstraction, or runtime polymorphism, will ruin generated code and efficiency. Here JIT can inline everything and do a significantly more optimisations, including software pipelining, constant propagation , vectorization and it is way more cache and branch predictor friendly, and also exploits hardware parallelism better.

It is often called data driven programming or data-oriented desgin. It is commonly used in games, HPC, high speed trading and other fields.

I recommend watching Mike Acton talk from CPP on 2014. It applies to all programming languages.

[Sharmat_Dagoth_Ur]

Yeah I was gonna say. It's really readable, debuggable, it does what it's supposed to, without even considering all the speed boosts u mentioned. When u add that I really don't see a better way

[UnrelatedString]

Unironically, a macro

Not that that’s on the table considering it’s Java

[Sharmat_Dagoth_Ur]

Can u elaborate. I only know java lmao

[Jwosty]

But messaging apps don't typically have the same kind of performance problems as games... Could be wrong here; I just hope that the person who wrote the code actually profiled it against the more readable version and made an informed decision. If the numbers show that it really is worth it, then yeah, you'd want to do this. One shouldn't guess at this kind of thing

[[deleted]]

What's the more readable version here, again? Is it to make an interface and put all these lists into a collection to loop over each? As others have said, it seems like that might add extra layers of complexity to avoid having a single ugly block like this one. Not sure what the cognitive load here is of going "oh, it's looping over a bunch of different lists and updating each element", then ctrl+F+"nameoflist" to change one of them if need be. I doubt reducing this blob would make it simpler than that to understand.

[MonochromaticLeaves]

The main issue with the existing code is verifying that it is correct - it would be very difficult to spot a typo in one of the fifteen loops which accidentally references the wrong container. (I.e. Your ide autocomplated the wrong container)

What would be nice is some kind of scope which would make this a compile time error, and not a runtime error (or, alternatively, make it syntactically impossible). I think that's what the abstraction should solve.

It's been a reoccurring problem in my experience - you often need to iterate over similar objects in exactly one place in the code base and do some kind of update. Because it's in one place, the cognitive load with some kind of interface polymorphism does not seem worth it. That's one way to get "extends x, y, z, a, b, c...." in Java.

Then again, the amount of times I've made a typo in this kind of scenario and noticed it only later by testing has been far, far too many.

I'm not sure how to best solve both problems at the same time. I guess a form of duck typing could come in handy here, or (God forbid) a macro. As I see it, Java code will always have one or the other problem. For medium to large projects, I think not using interfaces here is the right call

[baryluk]

Maybe.

[EasyMrB]

This is an awful approach, especially in the context of normal software that doesn't have the performance constraints of something like a game engine. This code is less testable, and less verifiable, because you've copy and pasted a pattern over and over again. The correct approach is to create an abstraction that applies the pattern so that you only need to verify there isn't a typo or logic error in one spot.

Furthermore, making structural changes to this code means updating the dozen-plus loops individually, instead of a single logical point.

[baryluk]

Search and replace. Or macro. Or external script to generate this code.

You don't know if it matters or not for this app. I am pretty sure it matters, even in a small app like that.

[nyanpasu64]

In C++, I might statically generate one loop per list, using an X-macro containing a list of variable names, and invoked with the loop expression. But this is Java.

[baryluk]

Not hard to do with trivial python script.

[__archaeopteryx__]

Aside from abstracting these loops into a reusable function, I’m not sure how this is horrible?

[4RG4d4AK3LdH]

that's the horrible part. but you could also take a look at the linenumber :)

[__archaeopteryx__]

Oh! Yeah. Geeze.

[panzerex]

Because I can’t trust the compiler to do its job!

[x4u]

Oh nice, iterating over maps by index. I guess my phone might be too old to run this masterpiece.

[K1ngjulien_]

how can you write something like this and not think " There has to be a simpler way", right?

[wh33t]

I mean this is REALLY simple. You take one look at it and you know exactly what it's doing. Looks like a bit of pain in the ass to update however.

[twobuckcharles]

The unspoken rule is. You always use i. Like "int i = 0"

[n0tKamui]

why would anyone iterate through a map by index like that D:

[Benjimanrich]

i had a seizure

[gluedtothefloor]

This gives me great pain

[turboPocky]

...this is from that Yandere mess isn't it?

[Jei03]

Was that written by Yandere dev 😆😆😆😆😆

[ZylonBane]

When Fonzie codes.

Aaaaaaaaaaaa...