Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow Monkey used injector to generate CRISPR Charge #65552

Merged
merged 5 commits into from
Apr 14, 2022
Merged

Allow Monkey used injector to generate CRISPR Charge #65552

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
063e76e
Change DNA Console behaviour so it will accept CRISPR charge from unf…
MrNihil Mar 18, 2022
4695de7
Made code little more clean.
MrNihil Apr 3, 2022
fff2bd1
Update code/game/machinery/computer/dna_console.dm
MrNihil Apr 10, 2022
e3a5bad
Code refactor
MrNihil Apr 10, 2022
a287fa1
Update code/game/machinery/computer/dna_console.dm
MrNihil Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions code/game/machinery/computer/dna_console.dm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,8 @@
to_chat(user,span_notice("[capitalize(CM.name)] added to storage."))
else
to_chat(user, span_notice("There was not enough genetic data to extract a viable chromosome."))
if(A.crispr_charge)
crispr_charges++
if(A.research && A.crispr_charge)
crispr_charges++
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reading through the code surrounding this, and I noticed that it's rather deeply indented (tabbed to the right)
image
It'd be pretty cool if you could make if(used) if(!used), and then return ..(), to make things a bit easier to read,
See https://github.com/tgstation/tgstation/blob/master/.github/guides/STYLE.md#use-early-returns

S not a you problem, but it'd be nice to see the code made less crummy.

In a perfect world we'd also do something like if(!A.research), but that gets mildly messy, so maybe not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ill look into guidelines and try to figure something out. What I saw is adding another nested if statement vs checking A.research twice and went with the first option for code-readability, since this is my first try with DM (but its kinda close logic-wise to what I already know).

Regarding A.research I will try to look into what it means first, as I assumed it was always true (since there is no code explaining it needed actual research and it works without the R&D node, or so I think).

Thank you for the link and review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course. If you need any help feel free to bug me yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Right now I am deep in RL now and wont be available for at least one week sadly. But in the end I want to do something about this code :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LemonInTheDark I cleaned this method a little bit. I noticed it mentions recycling injectors in the comments, but that functionality is lacking. Let me know if I understood your advice correctly.

Additionally I dont know much about this mysterious A.research, as I couldn't find where it is set to true (and obviously it has to be set somewhere, otherwise activators wouldn't work at all). My guess is that this var is useless and could be deleted, but I need some more time to figure it out.

I think It will be better to push this bug-fix first and concentrate on proper behavior later, as it doesn't seem to be a major issue for now. I plan to add some quality improvements later and could mix it with some fixes, like Genetic scanner showing which mutations are active, showing chromosome number even when researched for more clarity, or proposing some improvements like UE+UI scanning, blood DNA scan (it is a genetic scanner, right?) and syncing DNA research with R&D server, so discovered mutations could be shared among synced DNA Consoles (no more disk jogging between geneticists).

qdel(I)
return

Expand Down