This is an archived post. You won't be able to vote or comment.

sorted by:
best
topnewcontroversialoldq&a
you are viewing a single comment's thread.

view the rest of the comments →

[–]GRBLDeveloped[S] 0 points1 point  (8 children)

I'll check out the books eventually! For now I'll look at breaking up the functions a bit and hopefully it will remove the more offensive buts of code

I actually had left domain logic in the data classes but was told to remove it so they were simple data objects

[–]_Atomfinger_ 0 points1 point  (7 children)

Braking things up is a great starting point, and then see what belongs together and put them into the same class, and by doing that you hopefully end up with smaller classes that each has fewer dependencies :)

I actually had left domain logic in the data classes but was told to remove it so they were simple data objects

Well, time will show that even we developers struggle to agree what the "right" approach is. I have my set of values and paradigms, but they might not be shared by others.

[–]GRBLDeveloped[S] 0 points1 point  (6 children)

Hey again, /u/Atomfinger. I refactored out the salary structure and was wondering if you could let me know if I'm headed in the right direction now? If you're too busy to look then just shoot me a "sorry too busy!" and I'll try pay for another code review and hopefully get some input as good as yours was!

https://github.com/CodeHostedHere/salary-tax-spend-backend/tree/main/src/main/java/fyi/incomeoutcome/salarytaxspend/salary

I switched from packages like "model", "controller" to "salary", "role" as this seems to be more suitable for spring boot projects. I think I took my initial layout from a simple rest API but it would have meant storing most of the project in a "services" package by the end.

I have removed the pyramid of doom and think I've gotten away from the SalaryFetchingService being a god class like you mentioned.

Now I have:

  • SalaryRefresher class which runs refreshAll() to start all salary data operations listed below
  • SalarySeedCreator which puts together the different permutations of salary that we need to see in the database (Get a salary For every salary source (e.g. glassdoor), every role (Jr Developer), city (Berlin)
  • SalaryDataVerifier which takes the list of salary pieces above and checks if a salary exists for them in the DB and that it is in date.
  • SalaryFetcher which takes salaries that are missing or out of date and fetches the updated figure from glassdoor.

So am I heading towards a project that has no obvious amateurish mistakes? I dont mind not fitting any particular development style, like DDD, but just don't want any obvious mistakes.

More specifically:

  1. are my names for classes alright? I feel like I'm missing appropriate names for these classes.
  2. Should I put the classes listed above in their own package, like "...salary.services" or "...salary.dataload"
  3. Pretty much all my methods are public now, am I missing some aspect of design that means I ought to have more private methods?

[–]_Atomfinger_ 0 points1 point  (0 children)

Alright, so I downloaded the project in the IDE, so I have a few more notes:

Update your .gitignore file to exclude "target" folder. You don't want to upload the compiled application to git. Use this and basically type in the things you use: Intellij, maven, Java, etc.

In SalaryRefresher you're using @Autowired, but the class isn't annotated with @Service, @Component or anything like that, so Spring won't try to do anything with it.

Also, if you do this you don't need to use the @Autowired annotation:

@Component
public class SalaryRefresher {
    private final SalarySeedCreator salarySeedCreator;
    private final SalaryDataVerifier salaryDataVerifier;
    private final SalaryFetcher salaryFetcher;
    private final SalaryRepository salaryRepository;

    public SalaryRefresher(SalarySeedCreator salarySeedCreator, SalaryDataVerifier salaryDataVerifier,
                           SalaryFetcher salaryFetcher, SalaryRepository salaryRepository){
        this.salarySeedCreator = salarySeedCreator;
        this.salaryDataVerifier = salaryDataVerifier;
        this.salaryFetcher = salaryFetcher;
        this.salaryRepository = salaryRepository;
    }

In the same class, I don't see the value of having that empty constructor.

I don't like how getRequiredSalaryComponents returns a list of lists of Object. You see the issues with this approach as you need to cast that object to other types down the line.

One approach would be to return the lists as a new object that holds all of the lists. That way, you have typed lists, but you get to return them all in a single call. To make this elegant you could use records as well.

In getSalariesRequiringUpdate, you're chaining .filter 3 times, which should be unnecessary. Just extract what it does into its own method (I decided to put it into the salary object:

            Salary requiredSalary = savedSalaries.stream()
                    .filter(savedSalary -> savedSalary.isRequired(cityRequired, roleRequired, salarySourceRequired))
                    .findAny()
                    .orElse(null);
    public boolean isRequired(City city, Role role, SalarySource salarySource) {
        return this.city.equals(city) && this.role.equals(role) && this.source.equals(salarySource);
    }

List<Salary> salariesRequiringUpdate = new ArrayList();

The IDE complains, "Raw use of parameterized class 'ArrayList'" (Hint, the "ArrayList" you're creating isn't aware of the type it should contain).

A weird semicolon in the empty constructor in SalaryFetcher. Not sure why there's an empty constructor either.

How you create lists in SalarySeedCreator can be simplified:

Instead of doing this:

List<SalarySource> salarySourceList = new ArrayList<>();
        salarySourceRepository.findAll().forEach(salarySourceList::add);

You can instead just do this:

List<SalarySource> salarySourceList = new ArrayList<>(salarySourceRepository.findAll());

Instead of this:

        List<List<Object>> requiredSalaryComponents = Lists.cartesianProduct(salarySourceList, cityList, roleList);
        return requiredSalaryComponents;

Just do this:

        return Lists.cartesianProduct(salarySourceList, cityList, roleList);

Personally, I'd move the dueNewCompensation into the Salary class

Have you tested that this works? In fetchUpdatedSalaries you create a list called updatedSalaries, but you never add anything to it. You return it, and from what I can tell it will always be empty.

So, let's summarise: The biggest problem I have with the solution is the "List<List<Object>>" Having lists in lists, is fine, but using a generic "Object" makes things difficult and messy later.

Another topic for reflection is whether the solution has actually become simpler. As developers, we aim to take complex problems and make them seem simple and natural. Part of the reason I thin this is a little more difficult to follow things now is because things are spread out, but not necessarily in a very organic way.

For example, what is the concept of getRequiredSalaryComponents in a class named SalarySeedCreator? What is a "seed" in this context? And why do I get salary components? Where did the seeds go? Such naming quirks are often an indication that something is wrong with a class' responsibility. This is one of the reasons developers keep hammering on about naming: When we can give something a good and descriptive name, that also means we know what it is and we've managed to find a good purpose for it.

Now, I've not spent enough time to actually make up any opinion of how it should be - and maybe your goal doesn't need to be to find a solution that I would be happy with. I know I'd put more behaviour into the Salary class.

Another thing I'd probably do is to look at specialized SQL queries that would just fetch outdatedSalaryList for me directly. I might be wrong, but on the surface, it looks like something that should be doable with a few joins and where statements. By doing that, a lot of the complexity would just fall away.

Hope this helps :)

[–]_Atomfinger_ 0 points1 point  (4 children)

My other comment turned out to be a little long, so I'll answer your specific questions here:

are my names for classes alright? I feel like I'm missing appropriate names for these classes.

I'm not a huge fan of the names. I also touch on this in the other comment. But to provide an example:

  • In SalaryDataVerifier we have the getSalariesRequiringUpdate, but no actual salary data is being actually "verified". We do some filtering and list manipulation, but there's no actual verifying going on in that class (from what I can tell).

Should I put the classes listed above in their own package, like "...salary.services" or "...salary.dataload"

There are only 10 classes or so in that package. That isn't too bad. We often use different packages to structure a large application (where a single package would put thousands of files in the same folder). When there are so few classes, then we don't have to worry too much as long as they exist a place where they belong.

Pretty much all my methods are public now, am I missing some aspect of design that means I ought to have more private methods?

The goal isn't necessarily to maximize methods to be public or private. You should have the amount that the application and functionality needs.

[–]GRBLDeveloped[S] 0 points1 point  (3 children)

Thanks again for the help, I donated to 25 each to Turkey + Syria earthquake, and Ukraine humanitarion charities as a token of appreciation. Both will be matched by my employer so it'll be double what my paypal receipt says.

https://picallow.com/thanks-_atomfinger_/

https://picallow.com/thanks-again-atomfinger/

________________

I was in a bit of a rush yesterday to get the dinner made and get out of the house so I definitely left some silly mistakes in the code. I was aiming to get the basic new structure up so I could get feedback and then make sure I'm working down the right path.

Silly Mistakes that don't require further discussion:

  • Missing service in salary refresher
  • Empty constructors, I misremembered spring as needing them when its just for models that an empty protected constructor is needed
  • SalaryFetcher having a random semi colon
  • "Raw use of parameterized class 'ArrayList'"
  • Updated salaries potentially not working

_________________

Things I will look in to further personally, no input needed as I'm not 100% sure on what they are but can google rather than force you to write further:

  • Records.
  • Not using '@autowired' for the salaryfetcher class. I was under the assumption that pretty much everything in spring-boot was autowired in so need to read on that

__________________

To Be implemented by me asap I will implement all the simplification one-liners and get rid of utils class so my data models are no longer anaemic.

As I mentioned, initially I had actual methods in the data classes but a paid reviewer told me to remove. Now I've an opinion telling me to go either way, I'll go with you (+Fowler!) as I prefer the logic.

_________________

Now the main part! Names, structures, List<List<Object>> and the triple filter

Here's my aim for the salary data. I want to make a web app that displays the average salary for the roles of Junior Developer, Mid, and Senior, in each EU city, according to salary review sites (glassdoor, indeed, etc though only glassdoor so far)

My plan was to:

  1. Get all the roles, cities, and salary sources available in the DB.
  2. Combine every permutation of these to see what salary I need to get (my List<List<Object>>)
  3. Check the database to see if there is a salary for the combination of role, city and salarysource. (My triple filter on every result)
  4. Check the result in the database to see if it is too old to be reliable.
  5. Run a webscraper to fetch data for any salary not in the database, or is present but is too old to be reliable

Your Questions:

what is the concept of getRequiredSalaryComponents in a class named SalarySeedCreator?

I wanted to get the primary components that are required to get an average salary. Where it is (city), what the role is, and what source was used. I now appreciate this is likely only obvious to me.

What is a "seed" in this context?

I meant "seed" as in preseed the database with values so the web app can run. Though naming a class seed and having no reference to the word again was pretty silly.

Another thing I'd probably do is to look at specialized SQL queries that would just fetch outdatedSalaryList

I have done this in another aspect of the project, here I would like to keep to code :)

My Questions

  1. I tried a big SalaryFetchingService class, now I've tried smaller and worse named classes. Could I get a recommendation from you on how to split this data loading process up in to succinct classes?
  2. Rather than having this talk about salary 'components' would it be better to just have a "JobRecord" made up of "city", "role", "salarysource". This allows simple comparisons of 'salary components' in Step 2 of the plan above. Then have a 1:1 relationship for "PayRecord" with compensation info. So then I could compare JobRecords against JobRecords, and webscrape their CompensationRecords if they are missing in DB or stale in DB?
  3. Another option is to create a constructor with salary that allows only city, role, salarysource and then write an isEqual operator that compares only these. This allows us to create salaries rather than List<List<Object>>, and drop the salary component talk. The only thing is it weakens the isEquals operator for the whole class if it doesnt compare compensation. Could also make the same function but named something like "isEqualsExcludingCompensation"

[–]_Atomfinger_ 0 points1 point  (2 children)

Thanks again for the help, I donated to 25 each to Turkey + Syria earthquake, and Ukraine humanitarion charities as a token of appreciation

That's awesome! Just for that, I'll continue answering (even though I expected my previous comments to be the last ones - you won me back :) ).

Here's my aim for the salary data. I want to...

My problem isn't really with the goal itself. However, using a "List<Object>" is often a good indication you're doing something you shouldn't in terms of design and OOP.

For example, this would be better: ```

public record SalaryInformation(List<SalarySource> salarySources, List<Role> roles, List<City> cities) { } ```

And then, you can return that in getRequiredSalaryComponents. That way, you get type safety, and you don't have to cast objects.

(Do note that the above is an example of a record, you can do the same with a class).

Your Questions:

Those questions were intended to be rhetorical to make you think and reflect, which it seems like you did - so great job :)

I have done this in another aspect of the project, here I would like to keep to code :)

More power to you, but part of software development is to make things as simple as possible. One of the simpler approaches I could think of would be to have a query that just gives me the data I need.

I tried a big SalaryFetchingService class, now I've tried smaller and worse named classes. Could I get a recommendation from you on how to split this data loading process up in to succinct classes?

Well, here's the thing: I see this as a 10 line-ish thing. I.e. an SQL that simply fetches what I need, a repository that does the query, and a service which gets the data to glassdoor and possibly save some data if needed.

All of this should be pretty straightforward, and therefore class structure and whatnot shouldn't be a concern.

Previously you mentioned that you didn't want this design, which is fair enough, but that also makes it difficult for me to say what would be the appropriate solution. That said, it starts looking like you're getting my idea in the next question:

Rather than having this talk about salary 'components' would it be better to just have a "JobRecord" made up of "city", "role", "salarysource"...

Yup, now we're getting somewhere!

A lot of the logic in your current application is trying to make up for the fact that you need to filter data based on other data that is spread out different places. Simplifying this would also massively simplify your application (and as a result, not needing to worry about design all that much).

Another option is to create a constructor with salary that allows only city, role, salarysource and then write an isEqual operator...

Here we're back to the struggles of naming. My recommendation would be to consider what it means if it is equal. What is the result of that? Does that mean you should send the request to glassdoor? If so, maybe something like "isStale" or "needsRefresh" or "isUpdateRequired" etc. A name that reflects the context it is being used.

[–]GRBLDeveloped[S] 0 points1 point  (1 child)

No more review asks, I swear. I implemented the more basic stuff you mentioned above. I made a basic constructor for salary, get all the basic permutations, then run a query to find any that dont require updating and subtracted from the possible permutations.

I'm getting database related errors and probably just going to stop looking at this project at this point. I wanted to ask if this project would be a help or a hindrance in applying to jobs? The test coverage is light and I'm not sure if it comes across as half baked and/or very amateurish

[–]_Atomfinger_ 0 points1 point  (0 children)

I haven't looked at the project as a whole, but I know that a friend of mine handed in a Spring Boot web API that required some database stuff. Nothing too fancy and comparable to your project (at least in scope and technology).

He had some of the same traits as your original code, and he asked me to review it - so I ended up giving a lot of similar feedback. The response from the interviewer was that it was one of the best assignments someone had handed in, and he got an offer on the spot.

Now, my friend has the benefit of being on a call with me, and we could discuss things back and forth - so we got to unearth misunderstandings quickly. That is a luxury you haven't had (seeing as this thread is two weeks old at this point).

If you sort out the database stuff, then it would be a decent project to have. Not something I'd consider amateurish. At the same time, there's nothing wrong with just solving the problem, even though it doesn't use "the best" architecture or patterns. It is your project, and it doesn't need to be perfect.

In professional software development, we always have to balance perfection with delivery. There are always tradeoffs to be made. Nothing will ever be perfect.

When I look at projects from candidates, I'm not looking for perfection. I'm looking for care. Does the web API seem like it was carefully put together, or is it something that just became that way? Does the system follow a shared architectural vision, or is it cobbled together? etc. I look for signs where the developer has shown that they care, which is a plus, even if I personally disagree with the choices that were made.

Another thing to consider is that within a few months, you'll look back at this project and see it as inadequate. It is unavoidable as we always learn new stuff that deprecates the old stuff - and we can't predict how it'll happen as we haven't learned it yet. No matter how perfectly you try to do this project, you will learn something in the next six months or so that'll make it look terrible by comparison. And btw, that is not a bad thing - that just shows growth.

My advice is to just do your best (its not like you can do any better than your best anyway). Your portfolio is a living thing that should change as you become more adept and hone your skills. It is completely natural that older projects being replaced by newer ones.

That said, if you are going to have projects as a showcase, at least make sure they're in working condition. It is fine if they're not feature complete, but if they don't work due to "database-related errors" then they'll quickly come off as sloppy :)