Hot answers tagged

36

Just a philosophical answer here. I think you're asking the wrong question. You shouldn't ask yourself "is my SQL code safe or vulnerable?" This is too hard to answer in any individual case, and coming up with the right answer could depend on a lot of contextual things such as the system configuration, the whole program structure, etc. You don't want to ...


18

Your code is vulnerable to SQL injection! Use something like this to prevent it: cursor.execute("INSERT INTO table (state_int, turn_left, turn_right, move_forward) VALUES (?,?,?,?)",(database.records_table_name)) Note that you cannot (or you shouldn't) enter the table in like this: query = '''INSERT INTO %s...''' Ideally the table should be hard coded. ...


15

for(int i = 1; i <= 19; i++) { i = ( i%2 == 0 && i!=19 ) ? i+1 : i; [waitFrames addObject:[[CCSpriteFrameCache sharedSpriteFrameCache] spriteFrameByName:[NSString stringWithFormat:@"lev_wait2%04d.png", i]]]; } Holy Toledo. First of all, if you must update the iterator within a for loop, let's leave the update statement empty: ...


13

There's no way for us tell, from reading this fragment of code, whether there is a SQL injection problem. On the one hand, the query is constructed using string interpolation, which is risky, but the interpolated string is database.records_table_name and we can't tell whether this comes from an untrusted source. (You say in the post that it doesn't, but ...


11

I'm surprised nobody mentioned the code being vulnerable to SQL injection attacks: (XKCD source) Using string formatting and string concatenation to construct SQL queries is, first of all, unsafe and also, fragile - you will have to handle the argument types, balancing and escaping quotes yourself manually. Instead, you should parameterize your queries - ...


10

Initializing should be done automatically from your constructor. Right now you're exposing it as a public method that may or may not get called: this will guaranteed cause problems when someone forgets to initialize. Considering it doesn't add value in the first place I would just call it automatically in your constructor. Use comments to convey why you're ...


10

Reflection is slow. That line probably doesn't help your performance : var model = (T)Activator.CreateInstance(typeof(T)); I notice that you always create your instance with parameterless constructors. So you could add the type constraint new() in your class. That means you could do : var model = new T(); That'd be faster and well... clearer. You also ...


10

You could try doing the filtering at the database level. Something like: sql = "SELECT COUNT(*) FROM mydata WHERE myTextColumn MATCH %s" for word in wordList: db = cursor.execute(sql, word).fetchall() dicoVolume[word] = db[0][0] This would have the same word boundary caveats mentioned by Ludisposed, but it would save retrieving the entire table. ...


9

I exposed the database connection and database cur as instance variables. Is that a good practice? Having a cursor defined as an instance variable is a common practice. The problem though here is that you don't have a way to close the cursor and the connection itself in case of an error. In situations like this, having "context manager" capabilities on your ...


8

Some things that jump at me for the first look. Your DataType should definitely not be a String but some Enum. This makes passing invalid arguments to the function almost impossible. something like the following should do: public enum DbDataTypes{ VARCHAR, TIMESTAMP, LONG, INT, DOUBLE, DATETIME, [...] } I personally prefer to have the overloaded ...


8

I have just had a similar experience using Another Python SQLite Wrapper (APSW), which is an alternative to pysqlite that tends to be a bit faster. The PRAGMA options make practically no difference on a database of ~2 million rows (6 columns, also doing some string formatting on the values being entered): 753,347 rows/minute with all PRAGMAs at default (i.e....


8

All in all, this code is great. I don't see much in the way of problems. There are a few nit-picks: } catch (ClassCastException e) { throw new ClassCastException(activity.toString() + " must implement OnFragmentInteractionListener"); } The above code takes one ClassCastException and swaps it with another. Is it necessary? If ...


8

Is this a good design? No, not really. Some of the tables are not well designed. Take a look at the responsibilities of the users table and the media table. The media table has one clear responsibility: store the attributes of media items. Every record is self-contained, complete. You will probably never update the records in this table: you will only ...


8

When it comes to SQLite, I like to first wrap everything related to database into its own class: class SqliteDB: def __init__(self): self.parent_directory = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) self.db_path = os.path.join(self.parent_directory, 'db.sqlite3') self.db_connection = sqlite3.connect(self.db_path)...


8

There's two ways you can go about this. Stick with your current way, or use SQL more. Current way I'd change your for loop to assign to one value. I'd make a function that tells you if the values have the flag. I'd use *value to tuple unpack into the write_cursor tuple. And so would get something like: from datetime import datetime as dt def get_flag(...


8

SQL (and thus SQLite) support a variety of operators suitable (amongst others) in UPDATE statements. This means that you can simplify your whole loop into a single statement: query = ''' UPDATE Test SET Test2 = (Test2 + ?), Test3 = (Test3 + ?) WHERE Test1 >= 0 AND Test1 < 200 ''' c.execute(query, (200, 400)) Also you should take the habit to ...


7

[UIView animateWithDuration:0.03 delay: 0.0 options: UIViewAnimationOptionCurveEaseIn | UIViewAnimationOptionBeginFromCurrentState animations:^{ flashView.alpha = 1.0f; } completion:^(BOOL finished){ CGSize s = [[...


7

I have never used SQLite either, but something is jumping at me in your code: the addAuthor() procedure owns the connection, which means if you're calling it 10,000 times in a loop you're going to be opening and closing 10,000 connections, which is ..well to be honest, nuts. Looking at the method's signature: Sub addAuthor(ByVal authorlnf As String, ByVal ...


7

The difficulty you are having in the Python code is just a symptom of the underlying problem here, which the design of the database schema. At the moment, you have multiple tables, one for each class, something like this: CREATE TABLE class1 (name TEXT, score INT, date TEXT); CREATE TABLE class2 (name TEXT, score INT, date TEXT); CREATE TABLE class3 (name ...


7

Obviously, you already know about SQL injection. So let's forget about that. Ideally, please do defend against it, it's easy enough that it's not worth doing it any other way. Code that is not public facing now, might be next week. Some other things: You use fetchall instead of fetchone. fetchall has to load all rows into memory (I assume), which is pretty ...


7

In geoload.py use with..as to ensure it is closed, even if any code in-between raises an exception: with open("locations.data") as filehandle: ... In geodump.py, this is a code-smell: for row in cur: ''' row[0]: target row[1]: address row[2]: lat & long ''' It just asks for you to do: for target, _, data ...


7

You can simplify geoload by using the requests package. In fact it says this in the urllib documentation: See also: The Requests package is recommended for a higher-level HTTP client interface. Doing this single handedly changes: print 'Resolving', address url = SERVICE_URL + urllib.urlencode( {"sensor": "false", "address": address}) print '...


7

DRY This repetitive code is an anti-pattern: blueTotal = 0 greenTotal = 0 redTotal = 0 yellowTotal = 0 Please define houses = 'blue green red yellow'.split() and then you can use array access to perform "the same action" across all houses: for house in houses: total[house] = 0 One could also assign total = collections.defaultdict(int), but ...


6

Your IUnitOfWork interface has mixed concerns with IFeedRepository; I don't think the GetFeedData methods belong in there. The way I understand UoW, it should be looking something like this: public interface IUnitOfWork { void Commit(); // save changes void Rollback(); // discard changes } ..and vice-versa; if IFeedRepository.Save() has the same ...


6

You aren't closing the connection inside of a finally statement which means that in the case your application hits one of those exceptions it will not close the connection. this isn't good. I recommend using a using block to surround the code that you need performed by a connection. something like this Sub addAuthor(ByVal authorlnf As String, ByVal ...


6

The code looks fine to me but it is a bit odd because the official docs recommend creating a SQLiteOpenHelper subclass to handle database interaction. And if you use one SQLiteOpenHelper instance across your application, it will be thread-safe so there's no need to add your own synchronization logic: public class DatabaseHelper extends SQLiteOpenHelper { ...


6

I was able to remove the second query and merge it with the first one like this, I don't know why I didn't try it like this to begin with, probably because I wanted to tread lightly on this shaky bridge. I still don't have a solid way of passing variables to the OPENQUERY, but that is a question perhaps for StackOverflow when I have more time to work on ...


6

Ouch.... you are not filtering the data you are selecting from the OPENQUERY source.... But, let's get some things straight first.... you say: either way I don't think it would have been clean and straightforward anyway Using OPENQUERY automatically excludes any pretext of 'clean and straightforward'. They are incompatible. So, as soon as you use ...


6

Your code is not working well, and the SQL statement is not formatted nicely either. Reformatting your code in to logical sections produces. SELECT CAST(followers_count AS DECIMAL)/ CAST(friends_count AS DECIMAL), screen_name FROM following ORDER BY CAST(CAST(followers_count AS DECIMAL)/ CAST(friends_count AS DECIMAL) AS INT) DESC LIMIT 25; SQLite ...


6

let redButton = UIImage(named: "red_button") as UIImage? let greenButton = UIImage(named: "green_button") as UIImage? let yellowButton = UIImage(named: "yellow_button") as UIImage? The as UIImage? is 100% unnecessary here. UIImage(named:) already is defined as returning this type. Adding as UIImage? just clutters your code. override func ...


Only top voted, non community-wiki answers of a minimum length are eligible