11
\$\begingroup\$

I have built a Twitter clone in Golang, using object orientation principles. I wish to know the design mistakes I have made.

package blade

import (
    "database/sql"
    "fmt"
)

type User struct {
    Name     string
    Password string
    *sql.Tx
}

//User is a normal user who can login, register, tweet, retweet, follow and unfollow
func Stream(username string, transaction *sql.Tx) []Tweetmodel {
    rows, err := transaction.Query("Select id, tweet from tweets where username=$1", username)
    if err != nil {
        panic(err)
    }
    defer rows.Close()
    tweets := make([]Tweetmodel, 0)
    for rows.Next() {
        var id int
        var msg string
        rows.Scan(&id, &msg)
        tweet := Tweetmodel{id, msg}
        tweets = append(tweets, tweet)
    }
    return tweets
}

func (u *User) Follow(usertofollow User) string {
    name := usertofollow.Name
    var username string
    err := u.QueryRow("SELECT name from users where name=$1", name).Scan(&username)
    switch {
    case err == sql.ErrNoRows:
        return "You cannot follow an user who does not exist"
    default:
        if u.alreadyfollowing(name) {
            return "You have already followed this user"
        } else {
            u.Exec("INSERT INTO follow(username, following) VALUES($1, $2)", u.Name, name)
            return fmt.Sprintf("You have successfully followed %v", name)
        }
    }
}

func (u *User) alreadyfollowing(usertofollow string) bool {
    res, _ := u.Query("SELECT * from follow where username=$1 and following=$2", u.Name, usertofollow)
    return (res.Next() == true)
}

func (u User) Login() string {
    var username, password string
    err := u.QueryRow("SELECT name, password FROM users WHERE name=$1", u.Name).Scan(&username, &password)
    if err == sql.ErrNoRows {
        return "There is no user with that name, please try again or try registering!"
    }
    if u.Password != password {
        return "Your password is wrong, please try again!"
    }
    return "Welcome to Twitchblade"
}

func (u *User) Register() string {
    var username string
    err := u.QueryRow("SELECT name FROM users WHERE name=$1", u.Name).Scan(&username)
    switch {
    case err == sql.ErrNoRows:
        u.Exec("INSERT INTO users(name, password) VALUES($1, $2)", u.Name, u.Password)
        return "Successfully registered"
    default:
        return "User exists with same name.Please try a new username"
    }
}

func (u *User) alreadyretweeted(tweetid int) bool {
    var id int
    err := u.QueryRow("SELECT id from retweets where original_tweet_id = $1 and retweeted_by = $2", tweetid, u.Name).Scan(&id)
    return (err != sql.ErrNoRows)
}

func (u *User) iteratedretweet(tweetid int) (bool, int) {
    var id int
    err := u.QueryRow("SELECT original_tweet_id from retweets where retweet_tweet_id = $1", tweetid).Scan(&id)
    return (err != sql.ErrNoRows), id
}

func (u *User) Retweet(tweetid int) (string, int) {
    if u.alreadyretweeted(tweetid) {
        return "You have already retweeted this tweet", tweetid
    } else {
        flag, originalid := u.iteratedretweet(tweetid)
        if flag {
            return u.Retweet(originalid)
        } else {
            var msg, originaluser string
            var id int
            u.QueryRow("select username, tweet from tweets where id=$1", tweetid).Scan(&originaluser, &msg)
            _, retweetid := u.Tweet(msg)
            u.QueryRow("INSERT INTO retweets(original_tweet_id, retweeted_by, retweet_tweet_id) VALUES($1, $2, $3) returning id", tweetid, u.Name, retweetid).Scan(&id)
            return fmt.Sprintf("Successfully retweeted tweet by %s", originaluser), retweetid
        }
    }
}

func (u *User) Timeline() []Tweetmodel {
    rows, err := u.Query("select tweets.id, tweets.tweet from tweets INNER JOIN follow ON (tweets.username = follow.following) and follow.username=$1", u.Name)
    if err != nil {
        panic(err)
    }
    defer rows.Close()
    tweets := make([]Tweetmodel, 0)
    for rows.Next() {
        var id int
        var msg string
        rows.Scan(&id, &msg)
        tweet := Tweetmodel{id, msg}
        tweets = append(tweets, tweet)
    }
    return tweets
}

func (u User) Tweet(msg string) (string, int) {
    var id int
    u.QueryRow("INSERT INTO tweets(username, tweet) VALUES($1, $2) returning id", u.Name, msg).Scan(&id)
    return "Successfullly tweeted", id
}

func (u *User) Unfollow(usertounfollow User) string {
    res, _ := u.Query("SELECT * from follow where username=$1 and following=$2", u.Name, usertounfollow.Name)
    if res.Next() != true {
        return "You do not follow this user"
    } else {
        u.Exec("DELETE FROM follow WHERE name=$1 and following=$2)", u.Name, usertounfollow.Name)
        return fmt.Sprintf("You have successfully unfollowed %v", usertounfollow.Name)
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Why do Login and Tweet operate on a bare user while the other methods use a pointer? I've read the basics but haven't seen this distinction yet. \$\endgroup\$ Oct 21, 2015 at 19:10
  • \$\begingroup\$ @DavidHarkness It was just a matter of preference, nothing more. It is actually better to use pointer methods as there is no copying involved. I was learning Golang at that point of time and went ahead with normal method itself. \$\endgroup\$ Oct 23, 2015 at 8:05
  • \$\begingroup\$ A non-pointer receiver cannot change the orginal object \$\endgroup\$ Apr 21, 2018 at 21:21

1 Answer 1

2
\$\begingroup\$

I am aware this is old but here goes:

  • Can you confirm that *sql.Tx.Query is a proper query builder that prevents sql injection attacks (I don't know).
  • You are correctly closing the rows using defer.
  • Your Follow function could do the down selection on the database using a where clause to determine if you are not already following without requiring two queries (to distinguish between non-existent user and already following, you would have to return a count of the unfiltered username as well).
  • DO NOT store a users password; instead have a random per user salt generated and stored to perform multiple (10 say) becrypt rounds to get a suitably secure hash.
  • You can probably reduce the number of retweet queries by using a slightly more complex version of the following approach
  • Your code implies discrete polling for tweets doing table searches; this gets inefficient quickly with a n * m * u load on your database (n ~ number of people, m ~ number of following per person, u ~ update rate)
    • A much lower processing load (but greater storage) way is to have sharded tweet and followed message tables containing a list of tweets for a users attention (referencing the original tweet entry) so that you only need to do a simple search of a single shard of the followed tweet table for a given user to find all tweets for that users attention without doing table joins. Tweets of interest are pushed onto the sharded followed tweet tables when the original author tweets. The tweet and followed message tables are sharded by the tweeting user and following user respectively.
  • You could have a flag in the sharded user table that is set whenever there are new tweets to display, again avoiding excessive database loading by re-scanning an entire tweet table for new tweets.
  • You don't show how you would be doing app/web page updates; are you fast polling (bad idea), long polling (minimum fallback), a push notification service (which can get costly) or using websockets (good if it works for a given user but fiewalls and proxies often interfere) ?

NB: Reducing the sizes of tables, the number and complexity of queries to a minimum is essential to maintain system performance and minimise cost

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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