Featuresbranch #1
Conversation
… to receive, third to update the DB
jacsimas
added
invalid
| ps.setString(5, getterName); | ||
|
|
||
| int rows = ps.executeUpdate(); // returns 1 | ||
| System.out.println("Rows inserted: " + rows); |
There was a problem hiding this comment.
I would rename the variable to rowsInserted just so it is a little clearer. rows implies an entire row data structure with columns. rowsInserted implies just a number.
| System.out.println("Rows inserted: " + rows); | |
| System.out.println("Rows inserted: " + rowsInserted); | |
| String time = null; | ||
|
|
||
| String sql = "SELECT id, sender_id, sender_nickname, amount_sent, receiver_id, receiver_nickname FROM transactions_audit WHERE id = ?"; | ||
| // + customerId + " (id, nickname, money, password) " + "VALUES (?, ?, ?)"; |
There was a problem hiding this comment.
Avoid these kinds of comments. Generally speaking comments are a hot topic and I've seen both a lot of good and bad comments. The best kind of comments are no comments where the code is self-documenting. But if you believe you really need to add a comment read https://alexkondov.com/comments-the-good-the-bad-the-ugly and make your own conclusion. There is no hard rule here.
| try ( | ||
| PreparedStatement ps = connection.prepareStatement(sql)) { | ||
| ps.setLong(1, id); | ||
|
|
||
| try (ResultSet rs = ps.executeQuery()) { |
There was a problem hiding this comment.
You shouldn't nest try blocks. Let one trycatch exist and handle the cases there.
| } catch (SQLException e) { | ||
| System.out.println(e.getMessage()); | ||
| } |
There was a problem hiding this comment.
Useful for debugging, but I would advise not put these when writing production-ready code. You don't want your end-users see the bug message.
| receiver_id = rs.getInt("receiver_id"); | ||
| receiver_nickname = rs.getString("receiver_nickname"); | ||
| time = rs.getString("time"); | ||
| } |
There was a problem hiding this comment.
You can return this data immediately here. This would remove the need to instantiate these variables at the beginning of this function:
| } | |
| return new TransactionAudit(id, sender_id, sender_nickname, amount_sent, receiver_id, receiver_nickname, time); | |
| } | |
And you then can just throw an error at the end if something went wrong and there is nothing to return.
| ps.setLong(1, senderId); // customer_id | ||
| ps.setLong(2, receiverId); // friend_id |
There was a problem hiding this comment.
Either remove the comments or make the variables more meaningful:
| ps.setLong(1, senderId); // customer_id | |
| ps.setLong(2, receiverId); // friend_id | |
| ps.setLong(1, customerId); | |
| ps.setLong(2, friendId); | |
| addTx.setLong(1, senderId); | ||
| addTx.setLong(2, receiverId); | ||
| addTx.setLong(3, amount); | ||
| addTx.executeUpdate(); |
There was a problem hiding this comment.
Fluent setter here if possible.
| public void insertIntoTransferTotals1(int senderId, int receiverId, int amount) throws SQLException { | ||
| PreparedStatement upsert1 = connection.prepareStatement( | ||
| """ | ||
| INSERT INTO transfer_totals (customer_id, friend_id, sent_cents) | ||
| VALUES (?, ?, ?) | ||
| ON CONFLICT (customer_id, friend_id) | ||
| DO UPDATE SET sent_cents = transfer_totals.sent_cents + EXCLUDED.sent_cents | ||
| """); | ||
| upsert1.setLong(1, senderId); | ||
| upsert1.setLong(2, receiverId); | ||
| upsert1.setLong(3, amount); | ||
| upsert1.executeUpdate(); |
There was a problem hiding this comment.
Weird variable naming. Why the 1 here?
| public void insertIntoTransferTotals2(int senderId, int receiverId, int amount) throws SQLException { | ||
| PreparedStatement upsert2 = connection.prepareStatement( | ||
| """ | ||
| INSERT INTO transfer_totals (customer_id, friend_id, received_cents) | ||
| VALUES (?, ?, ?) | ||
| ON CONFLICT (customer_id, friend_id) | ||
| DO UPDATE SET received_cents = transfer_totals.received_cents + EXCLUDED.received_cents | ||
| """); | ||
| upsert2.setLong(1, receiverId); | ||
| upsert2.setLong(2, senderId); | ||
| upsert2.setLong(3, amount); | ||
| upsert2.executeUpdate(); |
There was a problem hiding this comment.
Weird variable naming. Would prefer to change it to something simpler. Why the 2?
| public void createTable() throws SQLException { | ||
| String sql = """ | ||
| CREATE TABLE IF NOT EXISTS test ( | ||
| id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY, | ||
| friend_name TEXT NOT NULL, | ||
| friend_status TEXT NOT NULL, | ||
| total_money_sent BIGINT NOT NULL, | ||
| total_money_received BIGINT NOT NULL, | ||
| since_when TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP | ||
| ); | ||
| """; | ||
|
|
||
| Statement st = connection.createStatement(); | ||
| st.executeUpdate(sql); | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
This is OK for this project, but this is also a good pivot for learning about database migrations.
You don't need to add it to this project, but you should learn about it once you got time. Same concept as here, only with ordered migrations + a reliable package does table schema changes for you.
| // DBController dbcontrol = new DBController(); | ||
| // | ||
| // ThreadTest R1 = new ThreadTest( "Thread-1"); | ||
| // R1.start(); | ||
| // | ||
| // ThreadTest R2 = new ThreadTest( "Thread-2"); | ||
| // R2.start(); |
There was a problem hiding this comment.
Be careful with comments like these. This is dead code which may stay dead forever. Either commit it uncommented or remove it, or leave a TODO at the top regarding what to do with it.
| private String user; | ||
| private int moneyInCents; | ||
| private String password; | ||
| private String realName; | ||
| private String email; | ||
| private String realName; | ||
| private String email; |
There was a problem hiding this comment.
This is a perfect class to replace with a record. See https://www.baeldung.com/java-record-keyword.
You set the data once when it instantiates, and it becomes permanently immutable.
| private String realName; | ||
| private String email; | ||
|
|
||
| public Customer(int i, String ss, int i1, String password) { |
There was a problem hiding this comment.
You should consider changing these parameters to be identical to their field names.
| package org.example.Other; | ||
|
|
||
| public class DataAutomation { | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
Be careful with leaving potentially unused classes.
| int senderreturnedId = dbcontroller.findCustomerByUsername(nameSenderCustomer); | ||
| int receiverreturnedId = dbcontroller.findCustomerByUsername(nameReceiverCustomer); |
There was a problem hiding this comment.
These variables should be camelCase
| boolean sendmoney = sendMoneyToSomeone(senderreturnedId, amountToSend); | ||
| if (sendmoney){ |
There was a problem hiding this comment.
Since it's a bool I assume it checks whether the transaction was successful... so I suggest changing the variable to reflect that:
| boolean sendmoney = sendMoneyToSomeone(senderreturnedId, amountToSend); | |
| if (sendmoney){ | |
| boolean isTransactionSuccessful = sendMoneyToSomeone(senderreturnedId, amountToSend); | |
| if (isTransactionSuccessful){ |
As it is the variable sendmoney is hard to understand just by reading it in isolation.
| } | ||
| } | ||
|
|
||
| // what if funds insufficient? add error handlers |
There was a problem hiding this comment.
Small trick you may not know:
| // what if funds insufficient? add error handlers | |
| // TODO: what if funds insufficient? add error handlers |
A TODO: as a comment prefix alerts your IDE to treat it as a TODO comment, which this one is. It is perfectly fine to leave these in unfinished parts of code. Don't overdo it, of course, but they are fine to use anyway.
| return true; | ||
| } | ||
| return false; | ||
| } // add isMoneyReceived method, splitting responsibilities and securing the transfers |
There was a problem hiding this comment.
| } // add isMoneyReceived method, splitting responsibilities and securing the transfers | |
| } // TODO: add isMoneyReceived method, splitting responsibilities and securing the transfers |
| dbcontroller.insertIntoTransferTotals1(senderreturnedId, receiverreturnedId, amountToSend); | ||
| dbcontroller.insertIntoTransferTotals2(senderreturnedId, receiverreturnedId, amountToSend); |
There was a problem hiding this comment.
It's hard to understand what the difference between insertIntoTransferTotals1 and insertIntoTransferTotals2 is. I suggest renaming the functions to reflect their differences better. Generally speaking cases like these should not be used.
|
|
||
| public class CustomerServices { | ||
|
|
||
| DBController dbcontroller = new DBController(); |
There was a problem hiding this comment.
A service should not use a controller. A controller should use a service.
This is the MVC pattern. See https://www.tutorialspoint.com/design_pattern/mvc_pattern.htm.
Services aren't part of MVC, but they are used to encapsulate business logic. A controller calls different services to do things. A service does not know anything about controllers.
In your case since you don't have a UI you won't have a View class anywhere, but that is fine, as you still follow MVC just with your views essentially being the terminal.
There was a problem hiding this comment.
Why are they called controllers anyway? They "control" the flow of code. They call each class one after the other, controlling the flow of data. It doesn't make sense that a handler or service would "control" when a controller is called in this case.
| @@ -0,0 +1,34 @@ | |||
| package org.example.Services; | |||
|
|
|||
| public class ThreadTest implements Runnable { | |||
There was a problem hiding this comment.
I don't understand this class. Is this a test? Or is it a class that you used while sandboxing multithreading?
| import java.sql.SQLException; | ||
|
|
||
| public class TransferHandler { | ||
| // CAN BE AN OBJECT RECEIVED FROM THE FRONTEND |
There was a problem hiding this comment.
I don't understand this comment.
There was a problem hiding this comment.
yeah, this class isn't used anywhere anyway so all good
| public class TransferHandler { | ||
| // CAN BE AN OBJECT RECEIVED FROM THE FRONTEND | ||
|
|
||
| DBController dbcontroller; |
There was a problem hiding this comment.
A controller should not be instantiated inside a handler. A handler should be instantiated inside a controller.
BTW if you're interested in naming things read this article written by a fellow Lithuanian: https://nikas.praninskas.com/javascript/2016/11/28/naming-things-handlers. It concerns itself with the overused naming of classes as etcHandler and etcHandler.handleSomething(...) in the industry. It's more as food for thought rather than an actionable request.
| String nameSenderCustomer = null; | ||
| String nameReceiverCustomer = null; | ||
| int amountToSend = 0; |
There was a problem hiding this comment.
You're mixing data transfer objects with service classes.
See https://martinfowler.com/bliki/AnemicDomainModel.html. This one may be a bit hard to understand at first. See https://dev.to/wallacefreitas/understanding-god-objects-in-object-oriented-programming-5636 to get an example of what a God Object anti-pattern looks like.
| int senderId = 5; | ||
| int receiverId = 3; | ||
|
|
||
| dbcontroller.insertIntoFriendships(senderId, receiverId); |
There was a problem hiding this comment.
You're not asserting anything here.
There was a problem hiding this comment.
I used it as frontend to insert info and test if data got updated in the database. Seemed simple at that time
| boolean isfriend = dbcontroller.checkIfHasFriend(7, 5); | ||
| // fix this, now it is only one way check | ||
| assertThat(isfriend).isTrue(); |
There was a problem hiding this comment.
This test is fine. You can add a second test to check if it's false, but generally speaking in terms of code coverage it should cover both cases already.
| // UserModel expectedUser = new UserModel(55, "bb", 100); | ||
|
|
||
| @Test | ||
| public void returnAuditRecordTest() throws SQLException { |
There was a problem hiding this comment.
You're not asserting anything here.
There was a problem hiding this comment.
Good job. But there's some things that should be changed.
Also, target directory has your compiled binaries which shouldn't be committed. Generally speaking anything that's binary or compiled (in some languages also called "minified") shouldn't be version-controlled as these files may contain sensitive environmental-dependent details as well as variables such as secret keys and passwords. They also take up a lot of space and can crash GitHub when viewing them from the browser, whereas actual .java files do not.
I suggest you learn to remove it and put target into .gitignore so git knows to ignore whenever you compile them. See https://docs.github.com/en/get-started/git-basics/ignoring-files and https://www.w3schools.com/git/git_ignore.asp on how to do that.
Co-authored-by: Ganiulis <91011496+ganiulis@users.noreply.github.com>
Co-authored-by: Ganiulis <91011496+ganiulis@users.noreply.github.com>
2 participants
Summary
Only the following commits matter:
It collects each transfer information (sender and receiver id, nickname, amount sent, date and time) that has been done by any of the customers, is inserted into a database table.
and
Finished working on friendships and friends transfers feature
Three separate postgreSQL tables created, one for friendships between customers, second for transfers audit only between customers who are friends.
One extra table that initially supposed to be only for friends, but now is for everyone - transfers totals table, it covers everyone's money totals sent and received separately to and from the different customer, by creating two rows for each transaction. for example:
sender_id | receiver_id | sent_cents | received_cents
-------------+-----------+------------+----------------
8 | 2 | 4000 | 0
2 | 8 | 0 | 4000
Testing
In order to test it there has to be a PostgreSQL database connected and 5 tables made.