Code reviews

[SpontaneousOrder]

Are the new classes called from multiple locations or just called once? If called once is there any plausible chance the functions could be reused directly in the future?

Unless the answer to either question is Yes you are not going to win a popularity context by refactoring working code regardless of how tulip it is...

Lets just assume only 1.
4 or 5 classes with a single responsibility each, and testable independently, surely is better than giant god class with 4 or 5 potential reasons to change.

Some people somehow see more classes == more complexity, whereas typically more classes == less complexity.

[SpontaneousOrder]

Just have to argue your case for the changes. Usually a good chance to improve things longer term if you do it well.

This is my plan. The role is a bit tulipty, so leaving the existing code better for the next guy is pretty much the only positive contribution i think i can make. Otherwise they should just hire a school leaver to just go with the flow.

(The code is very old for the most part, so it's not necessarily the fault of the guys currently there. It's just the lack of enthusiasm for improving it which strikes me)

[SpontaneousOrder]

Don't forget that because the code is better in the conventional way does not mean that that is what they wanted. As MS pointed out, they might have liked the code the way it was and found it easy to use etc.

In this particular situation i don;t think that's the case (it's a largish and pretty old system so i think a lot of it is as mysterious to them as it is to me). But that's a good reason to not go 'fixing' everything willy-nilly.

If that were the case I think i'd be struggling to maintain the will to live even more! At least now it's apathy rather than incompetence.

[kal]

Yeah.. sorry - i didn't mean to necessarily get a proper analysis. It just feels odd because it's my first contract, and it feels funny trying to throw my weight around when i'm only there for 6 months. I've been a consultant before but in those cases i've had the name of my employer behind me; whereas i know that people are often very suspicious of freelance conractors because there are so many poor ones.

In this particular case i have in mind it *could* have been a few lines fix, but those few lines were in tangle of tulipe (a chunk of which turned out to be redundant after i'd cleaned up a little) that needed some deciphering. So by doing the boy-scout thing and leaving it a bit better than I found it the logic is clear and easy to understand (i was able to delete literally dozens of lines of comments), unit tested (which it wasn't before), and more flexible insofar as any future tweaks are concerned (i.e each class now has a single/few reason/s to change). Hopefully when the selenium tests run everything will still look as it should do.

Unfortunately they don;t do any pairing at this place. I've found that when pairing it's easier to break down the culture of writing 1000 line classes because you can have a chat as you do it and explain why you think doing something is a better idea. Otherwise when it comes to a code review I think it;s much easier for someone to just think "this isn't what i'd normally do - it must be wrong".

Sounds like you had the best of intentions but unfortunately the permie noses you've put out of joint may come back to haunt you...I generally raise a technical improvement card for situations such as this rather then tack it onto the end of a bug fix or feature, gives time for the powers that be to decide is they want to do anything about the pile of tulip in question or are happy to live with it. Oh and I couldn't agree more about Pairing, a pity is not used more in development! ... I take it they're not an Agile shop?

[SpontaneousOrder]

Are the new classes called from multiple locations or just called once? If called once is there any plausible chance the functions could be reused directly in the future?

Unless the answer to either question is Yes you are not going to win a popularity context by refactoring working code regardless of how tulip it is...

I read that agin and i think i understand better where you're coming from (it works, you're changing it - why bother? why give me the hassle of reviewing it? why take the risk that it;s broken something).

That's a tough one. I was warned that the code was old and less than ideal at interview. I suggested that it's a case of doing the boy-scout thing where you leave any code you touch better than you found it and they agreed. But what's said in an interview doesn't necessarily match the reality.

I'm not sure what the point of me being there is otherwise though. Looking forwards to contract end either way

[SpontaneousOrder]

Sounds like you had the best of intentions but unfortunately the permie noses you've put out of joint may come back to haunt you...I generally raise a technical improvement card for situations such as this rather then tack it onto the end of a bug fix or feature, gives time for the powers that be to decide is they want to do anything about the pile of tulip in question or are happy to live with it. Oh and I couldn't agree more about Pairing, a pity is not used more in development! ... I take it they're not an Agile shop?

The difficulty i'm finding is that it's not a particular are of the code that is tulipe. It;s every single bit of code i need to work on. While i understand completely what you're saying, and agree, I also feel like a proper cowboy if I just go with the flow


They have agile teams, but i'm not in one of those so I can't comment on how well they do it. They build their own platform to provide their own service, so I can imagine that they'd be more likely to have a restricted view of the world and be bad at Agile. For all I know they could be great though. It might be worth me investigating because if there is a possibility of moving internally, rather than jumping ship in a few months, then it would be convenient.

[Freamon]

Add it to the technical debt register and raise it at the next review of said register. If such a thing does not exist, instigate it, and explain why its a good idea (i.e. why technical debt is a bad thing).

[NotAllThere]

Does anyone else experience dismay from permies code reviewing your code similar tho this?

I work on a fixing something in their 5 method, 1 thousand line Java source file, and in the process end up splitting it up into 3 or 4 extra classes, with all of the new classes tested and in total now more like 25 - 30 methods so that you can actually read the code in english instead of needing to learn to read the matrix.

Looks of dismay at code review time - apparently i've made the code more complicated!

Explain that while the code may look more complicated, because you've decoupled dependencies, and properly layered the application, you've made it more robust and it will be easier and quicker to maintain the future. You might also like to explain to any pointed-headed bosses, that "easier and quicker" and "more robust" is directly equivalent to "cheaper".

Properly structured code is a delight to work with.

[mudskipper]

Explain that while the code may look more complicated, because you've decoupled dependencies, and properly layered the application, you've made it more robust and it will be easier and quicker to maintain the future. You might also like to explain to any pointed-headed bosses, that "easier and quicker" and "more robust" is directly equivalent to "cheaper".

Properly structured code is a delight to work with.

Yes, but if you're asked to do a bug fix and you redesign the code completely, without first running it past whoever owns the code, you can expect to be met with looks of WTF?

[Platypus]

Yes, but if you're asked to do a bug fix and you redesign the code completely, without first running it past whoever owns the code, you can expect to be met with looks of WTF?

And you'd better be damn sure it works (no new bugs)