2
\$\begingroup\$

I live in the buildings that have 50 people in it and for each floor splits into 2 groups each group is from ~9 people. I'm trying to build an Xamarin app (IOS, ANDROID ,Win10,maybe web app) to make it easier to us to communicate.

Explaining the way i did my DB as it is:

The shopping table{ if someone by something the he can post it and keep track over how pay hem "payers"} i was planning to make a DB foreach group, i know that is bad data handling, so i made a collection ID for every group.i made 2 user variables so i can get the name of how did by and how need to pay (otherwise I end up only with one name)

I'm using Dapper with 2 list's: shopping and payers

Data Model

 public class Shoping
{
    public int ID { get; set; }
    public int UserID { get; set; }
    public string UserName { get; set; }
    public decimal Price { get; set; }
    public string Discretion { get; set; }
    public DateTime ProchDate { get; set; }
    public string PayURL { get; set; }
    public List<Payer> Payers { get; set; } = new();

}
public class Payer
{
    public int ID { get; set; }
    public int ShopID { get; set; }
    public string UserName { get; set; }
    public int UserID { get; set; }
    public decimal Price { get; set; }
    public decimal Payed { get; set; }
    public decimal Topay { get; set; }
}
public class User_collection
{
    public string ID { get; set; }
}
public class User
{
    public int ID { get; set; }
    public string CollectionID { get; set; }
    public string UserName { get; set; }
    public string Password { get; set; }

}

MySQL Tables

`shop`  (`ID`, `UserID`, `ProchDate`, `Price`, `PayURL`, `Discretion`)

`payer` (`ID`, `ShopID`, `UserID`, `Payed`)

`user`  (`ID`, `UserName`, `pas`, `CollectionID`, `phonenumber`)

`user_collection`(`ID`)

Finally this is my sql query and the c# code :

  public  Response<List<Shoping>> GetShop_GropID(string _CollectionID)
    {
     
        using (IDbConnection cnn = new MySqlConnection(c))
        {
            var p = new
            {
                CollectionID = _CollectionID
            };

            string sql = @"SELECT  S.* ,  us.UserName, P.* , up.UserName , s.Price , 
             (s.Price /(  SELECT  COUNT(DISTINCT payer.ID) FROM payer  WHERE ShopID = s.ID  ) - p.Payed ) AS Topay
                           FROM
                               shop S
                             JOIN payer P ON S.ID = p.ShopID 
                             JOIN USER up ON p.UserID = up.ID
                             JOIN USER us ON s.UserID = us.ID
                                     where 
                                           us.CollectionID = @CollectionID ;";
            var lookup = new Dictionary<int, Shoping>();

            _ = cnn.Query<Shoping, Payer, Shoping>
                (sql,
                (s, a) => {
                    Shoping shop;
                if (!lookup.TryGetValue(s.ID, out shop))
                {
                    lookup.Add(s.ID, shop = s);
                }
                if (shop.Payers == null)
                    shop.Payers = new List<Payer>();
                shop.Payers.Add(a);
                return shop;
                }  ,p).AsQueryable();
            var l = new List<Shoping>();
            l = (from s in lookup select s.Value).ToList();
            return new Response<List<Shoping>>(ResponseResult.Success, l);
        }
       
    }

Output Data

{
 "timestamp": 132640059769755780,
 "result": 1,
 "message": "Success",
 "content": [
  {
    "id": 1,
    "userID": 1,
    "userName": "Ror",
    "price": 100,
    "discretion": "ssss",
    "prochDate": "2021-04-13T00:00:00",
    "payURL": null,
    "payers": [
      {
        "id": 1,
        "shopID": 1,
        "userName": "Ror",
        "userID": 1,
        "price": 100,
        "payed": 0,
        "topay": 25
      },
      {
        "id": 2,
        "shopID": 1,
        "userName": "ror1",
        "userID": 2,
        "price": 100,
        "payed": 0,
        "topay": 25
      },
      {
        "id": 3,
        "shopID": 1,
        "userName": "ror1",
        "userID": 2,
        "price": 100,
        "payed": 10,
        "topay": 15
      },
      {
        "id": 4,
        "shopID": 1,
        "userName": "nan",
        "userID": 3,
        "price": 100,
        "payed": 10,
        "topay": 15
      }
    ]
  },
  {
    "id": 2,
    "userID": 1,
    "userName": "Ror",
    "price": 30,
    "discretion": "weq",
    "prochDate": "2021-04-05T00:00:00",
    "payURL": null,
    "payers": [
      {
        "id": 5,
        "shopID": 2,
        "userName": "nan",
        "userID": 3,
        "price": 30,
        "payed": 10,
        "topay": 20
      }
    ]
  },
  {
    "id": 3,
    "userID": 1,
    "userName": "Ror",
    "price": 30,
    "discretion": "weq",
    "prochDate": "2021-04-05T00:00:00",
    "payURL": null,
    "payers": [
      {
        "id": 6,
        "shopID": 3,
        "userName": "nan",
        "userID": 3,
        "price": 30,
        "payed": 10,
        "topay": 20
      }
    ]
  }
]
}

Is there a better way to structure the output data or the SQL query? Will I run into problems in the future?

When I use the query SELECT S.* , P.* , us.UserName, up.UserName , s.Price , I only get the username of the payer, but not the shopper. So I'm using SELECT S.* , us.UserName, P.* , up.UserName , s.Price .

The app will be user with groups so every 9 users share an indexed Collection ID I'm limiting the search with using 2 joins like so:

 Select * from shoping s
 JOIN USER u ON s.UserID = u.ID 
 JOIN user_collectionus C ON u.CollectionID = c.ID
 where C.ID = 'AAA';

How can I make this better or it's good as it is?

\$\endgroup\$
2
  • \$\begingroup\$ Welcome to CodeReview. Just to make sure: does your code work as expected? \$\endgroup\$ – Peter Csala Apr 27 at 15:27
  • \$\begingroup\$ Thanks. yes, but i have 2% experience with Data structure 😅 and when my DB get beger my code is super slow @PeterCsala \$\endgroup\$ – rynex akil Apr 27 at 15:32
1
\$\begingroup\$

There are quite a few naming issues in your code. You may want to think about installing a spell check extension in your IDE to help you catch these. I'm an appalling speller so I've got extra spell checks everywhere (IDE, browser etc.)

Naming is so important and the thing that is most important of all is that your naming is consistently applied. In your db you have PascalCase (UserName) and then some all lower (phonenumber). Find one that you like and stick to it everywhere - it really does help when reading the code back later. I won't keep on about that but I think you should read all your names again and check they are the best choice.

On to your SQL:

        string sql = @"SELECT  S.* ,  us.UserName, P.* , up.UserName , s.Price , 
         (s.Price /(  SELECT  COUNT(DISTINCT payer.ID) FROM payer  WHERE ShopID = s.ID  ) - p.Payed ) AS Topay
                       FROM
                           shop S
                         JOIN payer P ON S.ID = p.ShopID 
                         JOIN USER up ON p.UserID = up.ID
                         JOIN USER us ON s.UserID = us.ID
                                 where 
                                       us.CollectionID = @CollectionID ;";

That's unusual formatting of the query.

  • Why is USER uppercase?
  • Does the way it is written make it easy to scan/read?
  • Why are most, but not all, keywords upper case?
  • Great that you're using parameters!

Don't be afraid of line breaks in SQL, they really help with readability:

   string sql = @"
SELECT  
    S.*,
    us.UserName,
    P.*,
    up.UserName,
    s.Price, 
    (s.Price / (SELECT COUNT(DISTINCT payer.ID)
                FROM payer 
                WHERE ShopID = s.ID) - p.Payed) AS Topay
FROM shop S
JOIN payer P 
    ON S.ID = p.ShopID 
JOIN user up
    ON p.UserID = up.ID
JOIN user us 
    ON s.UserID = us.ID
WHERE us.CollectionID = @CollectionID;";

You could consider changing your aliases too. up could be payer_user and us could be shopper_user (just examples and evaluate whether you want/need to do this).

I don't think this query will scale badly as long as you have appropriate indexes in place. It's impossible to say what those indexes should be without being able to run the query with EXPLAIN. I would guess that Payer.ShopId would be a great candidate as it is likely to be highly selective (as I would guess there are few "payers" per shop). As with anything performance-related, measure, measure, measure!


Update:

Having read your comment about what the application is for, I think you can make things slightly easier for yourself. High level suggestion:

Purchase

  • Id
  • Purchaser (FK to user table)
  • ... interesting things about the purchase (e.g. payURL, date etc.)

PurchaseSplit

  • Debtor (FK to User table)
  • Purchase (FK to Purchase table)
  • Amount Owed
  • Amount Paid
  • Id can just be (PurchaseId, UserId) or add an Id column

User

  • Id
  • Name
  • etc.

When someone makes a purchase, you insert a row into Purchase and one row per "payer" into PurchaseSplit. At the same time, you calculate what each "payer's" share of the cost is. Note that in your DB solution, rounding can lead to the wrong amount calculated for everyone.

You can then update the PurchaseSplit when money is paid. This is a simple trade-off - you won't get history on payments but it's easy to implement and probably good enough for your case.

If the number of people splitting the purchase changes, you can recalculate the amount owed for all "payers" at the same time.

\$\endgroup\$
5
  • \$\begingroup\$ thanks!!, this is my second project if you sow my first one you will directly push Ctrl+Shift+del 😅. That's true I get usually confused by my variables name. I will install spell check extension and update all of my variables name. for the upper/lower case it's just a bad habit when i'm in focus mode. \$\endgroup\$ – rynex akil Apr 28 at 14:04
  • \$\begingroup\$ as i said to @BCdotWEB, I live in the buildings that have 50 people in it and for each floor splits into 2 groups each group is from ~9 people. I'm trying to build an Xamarin app (IOS, ANDROID ,Win10,maybe web app) to make it easer to us to communicate. the shopping table{ if someone by something the he can post it and keep track over how pay hem "payers"} i was planning to make a DB foreach group, i know that is bad data handling, so i made a collection ID for every group.i made 2 user variables so i can get the name of how did by and how need to pay (otherwise I end up only with one name) \$\endgroup\$ – rynex akil Apr 28 at 14:05
  • 1
    \$\begingroup\$ @rynexakil - I've added some thoughts on your database design. Hopefully, they make sense. \$\endgroup\$ – RobH Apr 28 at 19:03
  • \$\begingroup\$ They totally make sense. it is a better DB design. just a big concern 🙃 about Grouping people so as " ConnectionID", my app will have also other things like cleaning schedule and 6 different things. Do I have a better solution or I should keep joining the user table and limit my searching by the collection ID, if for ex( i need to select all the purchase of a specific group ID). something like indexing of partition the DB. just to avoid long search when the app grows. for now it's only 300 people (my bilding) if they like it then 7 buildings will use it so over 21000 people.thx for the help💜 \$\endgroup\$ – rynex akil Apr 28 at 20:20
  • 1
    \$\begingroup\$ @rynexakil - a problem with your connection id approach is that you will need to add a new group every time a person moves in or out. If you just update the group, the new person will be shown incorrectly against historical purchases. Write down some real world events and figure out if your data model supports them easily (or at all). \$\endgroup\$ – RobH Apr 29 at 6:15
1
\$\begingroup\$

Your query is rather baffling. You twice join with the USER table and yet I cannot see any reason for this.

This is what I cannot understand: (s.Price /( SELECT COUNT(DISTINCT payer.ID) FROM payer WHERE ShopID = s.ID ) - p.Payed ) AS Topay: you divide the price by the amount of payers? Why?


Honestly, I cannot figure out your data model. Why do Shoping and Payer have a UserName and also a UserID? Why do both have a Price? What is User_collection?

Are payers clients? Why would a client be specific to a shop? Why can't a client buy things at more than one shop?


You have business logic that is executed when the query is executed. This is bad, because it blocks your DB connection for no reason at all. Why not simply retrieve the data you need from the DB, and then work with the retrieved data outside of the using (IDbConnection cnn) block?


It also looks like this all happens within a method in a controller in your web API. This is bad. Controllers should be kept lean, you should move your logic to specific classes and use the likes of MediatR to connect them to your controller, and/or implement CQRS.

\$\endgroup\$
2
  • \$\begingroup\$ hi,Sorry for the confusion,I live in the buildings that have 50 people in it and for each floor splits into 2 groups each group is from ~9 people. I'm trying to build an Xamarin app (IOS, ANDROID ,Win10,maybe web app) to make it easer to us to communicate. the shopping table{ if someone by something the he can post it and keep track over how pay hem "payers"} i was planning to make a DB foreach group, i know that is bad data handling, so i made a collection ID for every group.i made 2 user variables so i can get the name of how did by and how need to pay (otherwise I end up only with one name) \$\endgroup\$ – rynex akil Apr 28 at 13:47
  • \$\begingroup\$ i'm looking into MediatR - Implementing CQRS and Mediator Patterns . it's first time i hear about it. \$\endgroup\$ – rynex akil Apr 28 at 14:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.