Calculate the Degrees of a Clock Hand Based on Input Time I'm a novice programmer. I started with Java years back and now mostly use VBA to automate tasks for my internships. I want to graduate from VBA and get into Python/R to properly automate tasks.
The below code is my first Python project. I would like any of you to point out how it can be improved.

The program is meant to take in a time in HH:MM format and print out at what degrees (from 12:00) each of the hands will be at. This was adapted from a common finance interview brainteaser guide.

I'm thinking that if I get some good suggestions for this code I'll go ahead and make one that includes a second hand.

input_time = input("Please input a time (HH:MM)")

time = input_time.split(":")

clock_degrees = 360
clock_hours = 12
clock_minutes = 60
clock_degreesperhour = clock_degrees/clock_hours
clock_degreesperminute = clock_degrees/clock_minutes

miunte_hand_degrees = int(time[1])*clock_degreesperminute

if (time[0] == "12"):
    time[0] = "0"

hour_hand_degrees = (int(time[0])*clock_degreesperhour)+((int(time[1])/clock_minutes)*clock_degreesperhour)

print("When the time is", input_time, 'the hour hand is at', hour_hand_degrees, "° and the minute hand is at", miunte_hand_degrees, "°")

Currently your code has user input, calculation and output all mixed up. This is OK as a first step (your code works, yay!), but the next step should be separating the calculation from the IO part. For this a simple function which takes the input already as parsed numbers will do:

def clock_angles(hour, minute, second):
    """Convert the time in hour, minute, second into angles of clock hands
       on a 12 hour clock, in degrees."""
    if hour >= 12:
        hour -= 12
    assert 0 <= hour < 12 and 0 <= minute < 60 and 0 <= second < 60
    degrees_per_hour = 360 / 12
    degrees_per_minute = degrees_per_second = 360 / 60
    return (hour * degrees_per_hour,
            minute * degrees_per_minute,
            second * degrees_per_second)

It even has a docstring describing what it does and some simple input validation.

Your IO can then be wrapped under a if __name__ == "__main__": guard which ensures it is not being run when you import from this script from another script. You can also use a f-string to make formatting the output slightly easier.

if __name__ == "__main__":
    input_time = input("Please input a time (HH:MM:SS): ")
    hour, minute, second = map(int, input_time.split(":"))

    hour_angle, minute_angle, second_angle = clock_angles(hour, minute, second)
    print(f"When the time is {input_time} the hour hand is at {hour_angle}°, the minute hand is at {minute_angle}° and the second hand is at {second_angle}°.")

I included the second hand here. Being able to parse the input without the seconds is left as an exercise.


Dijkstra's shortest path with max intermediate nodes limitation Suppose want to calculate the shortest path from one node to all other nodes, with the restriction of intermediate nodes cannot be more than a given number.

My major idea is, following how Dijkstra's algorithm works, with only the exception that when updating shortest path, if I see shortest path intermediate nodes exceeds the given number, I will skip the update. More details, refer to this line and len(self.shortest_path[shortest_node_so_far]+[n[0]])-1 <= max_path_stop

Here is my implementation, any advice on performance improvements in terms of algorithm time complexity, code bugs or code style are appreciated.

Test case is from wikipedia diagram (https://en.wikipedia.org/wiki/Dijkstra%27s_algorithm)

Source code in Python 2.7,

from collections import defaultdict
class Graph:
    def __init__(self):
        self.connections = defaultdict(list)
        self.visited = set()
        self.shortest_path = defaultdict(list)
        self.shortest_path_distance = defaultdict(lambda : float('inf'))
    def add_edge(self, edge):
        self.connections[edge[0]].append((edge[1], edge[2]))
        self.connections[edge[1]].append((edge[0], edge[2]))
    def calculate_shortest_path(self, cur, max_path_stop):
        self.visited.add(cur)
        self.shortest_path[cur] = [cur]
        self.shortest_path_distance[cur] = 0
        for n,d in self.connections[cur]:
            self.shortest_path_distance[n] = d
            self.shortest_path[n].append(cur)
            self.shortest_path[n].append(n)
        while (len(self.visited) < len(self.connections.keys())):
            shortest_so_far = float('inf')
            shortest_node_so_far = None
            for n,d in self.shortest_path_distance.items():
                if n in self.visited:
                    continue
                if d <= shortest_so_far:
                    shortest_node_so_far = n
                    shortest_so_far = d
            self.visited.add(shortest_node_so_far)
            for n in self.connections[shortest_node_so_far]:
                if self.shortest_path_distance[shortest_node_so_far] + n[1] < self.shortest_path_distance[n[0]] \
                        and len(self.shortest_path[shortest_node_so_far]+[n[0]])-1 <= max_path_stop:
                    self.shortest_path_distance[n[0]]=self.shortest_path_distance[shortest_node_so_far] + n[1]
                    self.shortest_path[n[0]]=self.shortest_path[shortest_node_so_far]+[n[0]]
if __name__ == "__main__":
    g = Graph()
    edges = [(1,2,7),(1,3,9),(1,6,14),(2,3,10),(3,6,2),(5,6,9),(5,4,6),(3,4,11),(2,4,15)]
    for e in edges:
        g.add_edge(e)
    g.calculate_shortest_path(1,2)
    print g.shortest_path
    print g.shortest_path_distance

If you apply your algorithm twice, as in

        g.calculate_shortest_path(1,3)
        ....
        g.calculate_shortest_path(1,2)

it would fail badly.

The reason is that visited, shortest_path, and shortest_path_distance are not, and cannot be, a property of Graph (especially visited). They are ephemeral properties of a particular traversal.

Questionably shortest_path and shortest_path_distance could be made properties of a vertex to allow for some optimization; I not quite sure it worths effort.


Efficiently matching pairs of elements and applying specific hover trigger/animation to each pair After scouring the interweb, I am having trouble finding an efficient solution to the following issue (the biggest problem was knowing what to ask):

I'm trying to find a way to write a more efficient version of the jQuery script found on the example site (also listed below). The biggest hurdle I'm facing is figuring out a way to dynamically match a trigger element (for example, .logo_wrap div.logo.img1) with its respective animating element (.sliding_titles .img1).

I've added numbers to the end of the divs that need to be matched, I just can't seem to find a way to properly match them, then apply a hover trigger to one and a slide-out animation to the other.

The example site I've created for this post only lists six objects, however there's another site that needs this to be done to 29 separate pairs of objects - which ends up being a ton of code.

Please see example site - which works fine. I just need to find a way to more efficiently write the script associated with it: http://redesign.mproven.com/sliding_example.html

Any and all help in this matter is greatly appreciated, thank you.

Here's a simple example:

Javascript (jQuery):

$(document).ready(function(){

$('.logo.img1, .logo.img2, ...etc,').fadeTo("fast", 0);

    $(".logo.img1").hover(function() {
    $(this).stop().animate({"opacity": "1"}, "500");
    $('.sliding_titles div.img1').stop().animate({opacity: 1, width: "500px", overflow: "visible" }, 400);
},
function() {
    $(this).stop().animate({"opacity": "0"}, "500");
    $('.sliding_titles div.img1').stop().animate({opacity: 0, width: "0px", overflow: "hidden" }, 1);
});

$(".logo.img2").hover(function() {

    ... etc.

});

HTML (in the order they appear in the body tag):

First are the content modules that slide out when its matched logo is hovered over:

YouTube slideout placeholder content

Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.

 ... etc ...

Then come the logo divs, which - when hovered - will trigger its respective content div to slide out (listed above):

    ... etc ...

`

$(function(){

    $('.logo').fadeTo("fast", 0);

    $(".logo").hover(function() {
        $(this).stop().animate({"opacity": "1"}, "500");
        $('.sliding_titles div#content-'+this.id).stop().animate({opacity: 1, width: "500px", overflow: "visible" }, 400);},
    function() {
        $(this).stop().animate({"opacity": "0"}, "500");
        $('.sliding_titles div#content-'+this.id).stop().animate({opacity: 0, width: "0px", overflow: "hidden" }, 1);
    });

`

YouTube slideout placeholder content

Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.

 ... etc ...

    ... etc ...


Parse weak ciphers out of an nmap.xml report I wrote the following code to parse some weak ciphers out of an nmap.xml report. I am wondering if there is a more elegant way to write this code in order to avoid the nested for loops with the double if loops. Also,  I had to create the two queues. There has to be a better way using list comprehension or a generator or something.

from xml.etree import ElementTree

md5Stack = []
shaStack = []
ip = []

with open('nmap.xml', 'rt') as f:
    tree = ElementTree.parse(f)
    for node in tree.getiterator('address'):
        ip = node.attrib.get('addr')
        for node in tree.getiterator('elem'):
            key = node.attrib.get('key')
            if key == "sha1" and ip not in shaStack:
                print ' %s :: %s' % (ip, key)
                shaStack += ip.split(",")
            if key == "md5" and ip not in md5Stack:
                print ' %s :: %s' % (ip, key)
                md5Stack += ip.split(",")

Example XML

<host starttime="1" endtime="1"><status state="up" reason="user-set" reason_ttl="0"/>
<address addr="1.1.1.1" addrtype="ipv4"/>

<elem key="type">md5</elem>
<elem key="bits">56</elem>

<elem key="notBefore">00:00+00:00</elem>
<elem key="notAfter">:00:00+00:00</elem>

<elem key="md5">34lk5jl4k5jlk34j5lk34j5lk</elem>
<elem key="sha1">234lk6j23lk6j2l3kj32lk4j3l4</elem>
<elem key="pem">BEGIN CERTIFICATE;END CERTIFICATE;</elem>
</script></port>
</ports>
<times srtt="98" rttvar="4" to="2"/>
</host>

I don't believe that this does what you want, for a few reasons.

First, I'm not 100% on interleaving ElementTree iterators, but this may pick up more elements than you want; for example, what happens when the document is something like

<host>
<address addr="1.1.1.1"/>
</host>
<host>
<address addr="2.2.2.2"/>
<elem key="md5"/>
</host>

It's at least plausible to me that both IP addresses would show up in the md5 list. Even if it doesn't, you should strive to use code that has clear semantics.

Second, your filters check that ip not in md5Stack but what you add to md5Stack is ip.split(',').

Let's try this:

from xml.etree import ElementTree

def extract_weak_ips(nmap_root):
    md5_ips = set()
    sha1_ips = set()
    hosts = list(nmap_root.iter('host'))
    for host in hosts:
        addresses = [address.get('addr').split(',')
                     for address in host.findall(".//address[@addr]")]
        if host.find(".//elem[@key='md5']") is not None:
            md5_ips.update(addresses)
        if host.find(".//elem[@key='sha1']") is not None:
            sha1_ips.update(addresses)

def print_ip_set(s, key):
    for ip in s:
        print ' %s :: %s' % (ip, key)

if __name__ == '__main__':
    parser = ElementTree.XMLParser()
    parser.parser.UseForeignDTD(True)
    md5_ips, sha1_ips = extract_weak_ips(ElementTree.parse('nmap.xml', parser).getroot())
    print_ip_set(md5_ips, 'md5')
    print_ip_set(sha1_ips, 'sha1')

I've extracted code into clear functions, and I'm using XPath queries so that the XML engine is doing the work of querying XML, not your application-specific code.

I think that in this case the list-comprehension approach is less readable than the code with for-loops, but if you really want list comprehensions, try this:

def extract_ips_with_key_type(nmap_root, key_type):
    query = ".//elem[@key='%s']" % key_type
    return set(apply(
        itertools.chain,
        (address.get('addr').split(',')
         for host in nmap_root.iter('host')
         if host.find(query) is not None
         for address in host.findall('.//address[@addr]')))


Periodically resizing large array I have a simple socket.io server written in node. Everything is working just fine, however I am curious whether what I am doing is acceptable.
I am especially interested in this line

listOfMessages = listOfMessages.slice(-1000);

Essentially what I am doing is each time a new user connects, I resize the array and send the newly connected user the last 1000 messages.

I do not want to let the array grow without bounds since the server can stay running indefinitely.

var listOfMessages = [];

io.on('connection', function(socket){
  console.log('a user connected (id=' + socket.id + ').');

  // This way list of messages doesn't grow larger than 1000...
  listOfMessages = listOfMessages.slice(-1000);
  // Loop over the list of messages and send them to the new connections
  for (index = 0; index < listOfMessages.length; index++) {
      socket.emit('chat message', listOfMessages[index]);
  }

  // When we recieve a message
  socket.on('chat message', function(msg){
    console.log('message: ' + msg + ' from client (id=' + socket.id + ').');
    socket.broadcast.emit('chat message', msg);
    listOfMessages.push(msg);
  });
});

Some feedback on whether this is acceptable or good practice is what I'm looking for.

From what I understand, your code only keeps at most 1000 messages. First, you need to move out 1000 to a variable and away from your logic. That way, it's visible and easily configured.

Next, there is a possibility that your list of messages will grow greater than a thousand because you only slice it off when a connection is made. What if people kept chatting, but nobody new connected? Your array will continually grow. I suggest you move that check to the "chat message" event.

Another way to do it is to just separate this slicing logic into something periodic, like run on intervals. That way, you don't have to slice every time. The drawback to this, though, is when people chat faster than you can slice off.

Now for the logic that keeps your history in check. slice is expensive because it does a shallow copy of your array. Having a full 1000 messages in the list, if a person connects, you'd have 2 arrays of length 1000 at one moment. I suggest you use shift when the count runs over 1000. Assuming this system is only a single process, pushing in the 1001th message should immediately shift off the oldest one. Still an expensive process, but it's a lesser evil.

Lastly, to get the latest 1000 messages, it's an overhead to send 1000 events down the line to the new user individually. I suggest you create a new event that sends all 1000 at once instead of individually.

Let's try optimizing your logic:

var listOfMessages = [];
var MAX_HISTORY_LENGTH = 1000;

io.on('connection', function(socket){

  // Send down all old messages at once to this connection.
  socket.emit('new messages', listOfMessages);

  // When we recieve a message
  socket.on('chat message', function(msg){

    socket.broadcast.emit('chat message', msg);
    listOfMessages.push(msg);

    // Only splice when we need to
    if(listOfMessages.length > MAX_HISTORY_LENGTH) listOfMessages.shift();

  });
});


Removing objects from a list that don't appear in a list of ID's I have a list of objects with each of them having an ID. I have a second list that is just a list of media ID's. I want to remove the objects from the first list that don't match a mediaID contained in the second list. What is an efficient way of going about this? The only way I could think of was to have a nested for loop but that seems inefficient.

Here is my implementation. Note that "items" is the first list, containing MediaDetailsItem objects, with each of these objects having a specific mediaId. The second list is mediaIds.

List<MediaDetailsItem> shortenedList = new ArrayList<MediaDetailsItem>();
                        for(MediaDetailsItem item : items){
                            for(String id : mediaIds){
                                if(item.getMediaId().equals(id)){
                                    shortenedList.add(item);
                                    break;
                                }
                            }
                        }
                        items.clear();
                        items.addAll(shortenedList);

You can do this without external iteration, if you are using java-8:

items.removeIf(item -> !mediaIds.contains(item.getMediaId()));

It's not necessary to create a new list, this will modify the existing one.


PHP pages that interact with MySQL Could someone review this PHP code to see if my pages are coded safely to not allow common attacks (SQL injection, XSS, etc.)? I would be thankful, of course, for anyone to point out other problems in my code as well.

// getpaste.php
<?php
$db_conn_config = parse_ini_file('../private/db_conn.ini');
$servername = $db_conn_config['servername'];
$username = $db_conn_config['username'];
$password = $db_conn_config['password'];
$dbname = $db_conn_config['dbname'];
$conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);

// Prepare sql and bind parameters.
$stmt = $conn->prepare("SELECT key_salt, aes_iv, cipher_text, mac_tag FROM pastes WHERE paste_id =:pasteid");
$stmt->bindParam(':pasteid', $_POST["pasteID"]);
$stmt->execute();

// Only output results if 1 row is found.
if ($stmt->rowCount() == 1)
{
    $row = $stmt->fetch();
    echo $row['key_salt'];
    echo "|";
    echo $row['aes_iv'];
    echo "|";
    echo $row['cipher_text'];
    echo "|";
    echo $row['mac_tag'];
}

$conn = null;
?>

// savepaste.php
<?php
$db_conn_config = parse_ini_file('../private/db_conn.ini');
$servername = $db_conn_config['servername'];
$username = $db_conn_config['username'];
$password = $db_conn_config['password'];
$dbname = $db_conn_config['dbname'];
$conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);

// Make sure that the values written to the database only include valid base64 characters.
// These are letters (upper and lower case), numbers, and /+=
$pattern = "@^[a-zA-Z0-9/+=]+$@";
if ((!preg_match($pattern, $_POST["pasteID"])) or
    (!preg_match($pattern, $_POST["keySalt"])) or
    (!preg_match($pattern, $_POST["aesIV"])) or
    (!preg_match($pattern, $_POST["cipherText"])) or
    (!preg_match($pattern, $_POST["macTag"])))
{
    echo "Invalid input characters.";
    exit(1);
}

// Depending on action variable, either do an insert or update.
if ($_POST["action"] == "insert")
{
    $pre_stmt = "INSERT INTO pastes VALUES (:pasteid, :keysalt, :aesiv, :ciphertext, :mactag)";
}
else if ($_POST["action"] == "update")
{
    $pre_stmt = "UPDATE pastes SET key_salt=:keysalt,aes_iv=:aesiv,cipher_text=:ciphertext,mac_tag=:mactag WHERE paste_id=:pasteid";
}
else
{
    echo "Invalid form action.";
    exit(1);
}

// Prepare sql and bind parameters.
$stmt = $conn->prepare($pre_stmt);
$stmt->bindParam(':pasteid', $_POST["pasteID"]);
$stmt->bindParam(':keysalt', $_POST["keySalt"]);
$stmt->bindParam(':aesiv', $_POST["aesIV"]);
$stmt->bindParam(':ciphertext', $_POST["cipherText"]);
$stmt->bindParam(':mactag', $_POST["macTag"]);
$stmt->execute();
$count = $stmt->rowCount();
echo $count;

$conn = null;
?>

// deletepaste.php
<?php
$db_conn_config = parse_ini_file('../private/db_conn.ini');
$servername = $db_conn_config['servername'];
$username = $db_conn_config['username'];
$password = $db_conn_config['password'];
$dbname = $db_conn_config['dbname'];
$conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);

// Prepare sql and bind parameters.
$stmt = $conn->prepare("DELETE FROM pastes WHERE paste_id = :pasteid");
$stmt->bindParam(':pasteid', $_POST["pasteID"]);
$stmt->execute();
$count = $stmt->rowCount();
echo $count;

$conn = null;
?>

Prepared statements prevent any form of SQL injection so that is not an issue with your code.

Although it may not be required in your case, I see no user control which would prevent anyone from filling your database with junk, or even deleting all records from it.

As for XSS, the code provided shows no protection from it in any way.  XSS protection implies data validation, sanitizing, and output escaping but none of those are done here.  You should refer to http://www.sitepoint.com/php-security-cross-site-scripting-attacks-xss/ or other similar tutorials to learn more on XSS protection.

Even if XSS wasn't a source of worry, you should never accept user input as is without extensive validation.

Another thing I noticed is that you're repeating the same code in all your scripts for the configuration.  For maintainability, that greatly justifies placing it in another script that gets included by them such as config.php which would include:

$db_conn_config = parse_ini_file('../private/db_conn.ini');
$servername = $db_conn_config['servername'];
$username = $db_conn_config['username'];
$password = $db_conn_config['password'];
$dbname = $db_conn_config['dbname'];
$conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);


Import multiple XML files into Excel I have adapted a VBA code to import all the XML files I have on a folder into Excel. I have tried to run the code with only two files and it takes about 30 seconds. Several minutes to import less than 20 files.

I need to import more than 200,000 files. Can you please help me on how to improve my code?

Sub ListFiles()
'DECLARE AND SET VARIABLEs
Dim ShellApplication As Object
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Set ShellApplication = CreateObject("Shell.Application").BrowseForFolder(0, "Please choose a folder", 0, OpenAt)
Path = ShellApplication.self.Path
Set ShellApplication = Nothing
[a3] = "XML"
[b3] = "Files"
'DEFAULT PATH FROM HIDDEN SHEET
Call ListMyFiles(Path, True)
End Sub

Sub ListMyFiles(mySourcePath, IncludeSubfolders)
    Set MyObject = New Scripting.FileSystemObject
    Set mySource = MyObject.GetFolder(mySourcePath)
    'With SearchXML
    Application.ScreenUpdating = False

'--------------------------------------------------------------------
    'FIND XML FILES ONLY, APPLY SEARCH CRIERIA, DISPLAY MATCHES ONLY
    For Each myfile In mySource.Files
        If Right(myfile.Name, 3) = "XML" Or Right(myfile.Name, 3) = "xml" Then 'IS XML?
            LastRow = ActiveSheet.Cells(Rows.Count, 2).End(xlUp).Row

 '-------------------------------------------------------------
            'IMPORT XML FILE
            Application.DisplayAlerts = False
            ActiveWorkbook.XmlImport URL:=mySource & "\" & myfile.Name, _
                ImportMap:=Nothing, Overwrite:=True, Destination:=Range("$B$" & LastRow + 1)
            Cells(LastRow + 1, 1) = myfile.Name

'------------------------------------------------------------
            'DELETE MAPS
            maps = ActiveWorkbook.XmlMaps.Count
            For i = 1 To maps
                ActiveWorkbook.XmlMaps(1).Delete
            Next i
        End If
    Next
    If IncludeSubfolders Then 'SEARCH SUBFOLDERS FOR SAME CRITERIA
        For Each MySubFolder In mySource.SubFolders
            Call ListMyFiles(MySubFolder.Path, True)
        Next
    End If
    'End With
    Application.ScreenUpdating = True
Application.DisplayAlerts = True
Sheets("XMLData").UsedRange.WrapText = False
End Sub

Not knowing what your XMl files are like or what you're trying to extract from them, this is mostly speculative:

Generally, you want to strip away as much unwanted data as early as possible in the process. Rather than trying to import an entire file to the worksheet every time, why not strip your document down to a NodeList, and then either target specific nodes (if you're only after certain data) or iterate over them and extract the data values into an array. Then you can print that Array to a worksheet.

I recently built a Tool to extract data from corporate accounts. It loads an XML doc (typically 100kb), parses the Node Tree, searches for specific nodes using XPath, extracts the displayed text of each and sticks it in an array that's printed to a worksheet every 500 lines or so. It grabs 5 sets of nodes from each document, and parses about 1 Million documents/hour, or nearly 300 documents / second.

Below is an extract showing the basic processing of loading, stripping, searching and retrieving.

Public Sub GetDirectorsFromFile(ByVal strFullFilename As String)

    Dim xDoc As MSXML2.DOMDocument
    Set xDoc = New MSXML2.DOMDocument

    Dim colDirectors As Collection
    Set colDirectors = New Collection

    With xDoc

        If .Load(strFullFilename) Then

            .setProperty "SelectionLanguage", "XPath"
            GetDirectorsFromXml xDoc, colDirectors

        End If

    End With

End Sub

Public Sub GetDirectorsFromXml(ByRef xDoc As MSXML2.DOMDocument, ByRef colDirectors As Collection)

    Dim ixItem As Long
    Dim xPathSearchString As String
    Dim nodes As MSXML2.IXMLDOMNodeList
    Dim nodeText As String

        xPathSearchString = "//*[contains(@name,""NameEntityOfficer"")]"
        Set nodes = xDoc.SelectNodes(xPathSearchString )

        For ixItem = 0 To nodes.Length - 1
            nodeText = nodes.Item(ixItem).text
            colDirectors.Add text
        Next ixItem

End Sub


Can this while loop be made cleaner Moved to Code Review as per comments received on https://stackoverflow.com/questions/20907502/can-this-while-loop-be-made-cleaner

Is there a way to make the following while loop a little more optimized? What bugs me in particular is the fact that I have to repeat code (closing buffers and returning a value) both inside and outside the if condition and I wanted to get opinions on whether there might be a better /more performance-oriented way to handle such code.

While I've posted the entire method, the part I'm more interested in comments on is how the while loop can be optimized. Ofcourse, comments on the remainder of the code are also welcome, but not essential.

    private String getRandomQuote(int lineToFetch)
        throws IOException {
    //1. get path
    AssetManager assets = getApplicationAssets();
    String path = null;
    path = getAssetPath(assets);

    //2. open assets
    InputStream stream = assets.open(path);
    InputStreamReader randomQuote = new InputStreamReader(stream);

    //3. Get BufferedReader object
    BufferedReader buf = new BufferedReader(randomQuote);

    String quote = null;
    String line = null;
    int currLine = 0;

    //4. Loop through using the new InputStreamReader until a match is found
    while ((line = buf.readLine()) != null) {
        // Get a random line number
        if (currLine == lineToFetch) {
            quote = line;
            Log.v("LINE", line);
            randomQuote.close();
            buf.close();
            return quote;
        } else
            currLine++;
    }
    randomQuote.close();
    buf.close();
    return quote;
}

Nice little system, I understand though why you think this code could be better.

I think there are a few things which would make a difference. The first is a try-finally block, and the second is a simpler loop

First, the try-finally:

    InputStream stream = assets.open(path);
    try {
    //3. Get BufferedReader object
        BufferedReader buf = new BufferedReader(new InputStreamReader(stream));
        .... do things with the buffer.
    } finally {
        stream.close();
    }

This is a bit of a pre-Java7 cheat I use. It is safe to close the inner stream, and let the outer Readers (BufferedReader,InputStreamReader) not be explicitly closed. In Java7 I would put them all in a try-with-resources, but, Android, alas!.

With the above code you don't need to worry about closing the stream at all inside the try block.

OK, so now the try-block is simple, I would put two conditions (the readLine first!) in to the read-loop:

String line = null;
int currLine = 0;

//4. Loop through using the new InputStreamReader until a match is found
while ((line = buf.readLine()) != null && currLine < lineToFetch) {
    currLine++;
}

// OK, at this point, line is either null,
// or the actual line to fetch.... Whatever it is, return it,
return line;

Putting it all together, I would have something like:

private String getRandomQuote(int lineToFetch)
        throws IOException {
    //1. get path
    AssetManager assets = getApplicationAssets();
    String path = null;
    path = getAssetPath(assets);

    //2. open assets
    InputStream stream = assets.open(path);
    try {
        //3. Get BufferedReader object
        BufferedReader buf = new BufferedReader(new InputStreamReader(stream));
        String line = null;
        int currLine = 0;

        //4. Loop through using the new InputStreamReader until a match is found
        while ((line = buf.readLine()) != null && currLine < lineToFetch) {
            currLine++;
        }

        // OK, at this point, line is either null,
        // or the actual line to fetch.... Whatever it is, return it,
        return line;

    } finally {
        stream.close();
    }
}


Should view related methods be included in models? class Submission < ActiveRecord::Base

  def get_status_css()
    if self.status == IN_PROGRESS
      "info"
    elsif self.status == FAILED
      "error"
    elsif self.status == COMPLETED
      "success"
    else
      "warning"
    end
  end

end

Submission is a model class and it has a method that returns css variables based on its status. Is this correct in the MVC architecture? I was thinking that since get_status_css is a method that is related to view, it shouldn't belong to a model class. Perhaps it should be put in the controller?

This could should not be in either the model or the view.  Rather is should be contained in the submission_helper, or if you really wanted to look into making it less procedural and more OO, then it should be handled by a presenter, a pattern in rails that steps in between the model and the view.

Jeff Casimir has developed a presenter for RoR called Draper (https://github.com/jcasimir/draper) which is definetly worth checking out, but in the mean time, the proper place for this is a helper method.

Its best to avoid logic in your view code, as its likely to change, and be distributed amongst many files, making it hard to maintain.  Putting it in a helper at least allows for a single source of that knowledge, so that if it has to change in the future it is easy to update.

Hope this helps.


POCO helper class refactoring I have a helper class that I am using to return the selected items in a number of IEnumerable POCO collection but I'm not happy with the implementation. One of the reasons for using this method is that the collections area returned by RIA services as ReadOnlyObservableCollections and I don't believe they support LINQ but may be wrong.

Is there a way to refactor these three methods using generics?
Are there any other improvements that I can make?

public class ModelHelper : IModelHelper
{
    public IList<Int32> GetSelectedResults(IEnumerable<FaultAuditScoresDto> scores)
    {
        IList<Int32> selected = new List<Int32>();

        foreach (FaultAuditScoresDto s in scores)
        {
            if (s.IsSelected)
            {
                selected.Add(s.ID);
            }
        }

        return selected;
    }

    public IList<Int32> GetSelectedResults(IEnumerable<ZonesDto> zones)
    {
        IList<Int32> selected = new List<Int32>();

        foreach (ZonesDto s in zones)
        {
            if (s.IsSelected)
            {
                selected.Add(s.ID);
            }
        }

        return selected;
    }

    public IList<Int32> GetSelectedResults(IEnumerable<FaultAreasDto> faultAreas)
    {
        IList<Int32> selected = new List<Int32>();

        foreach (FaultAreasDto s in faultAreas)
        {
            if (s.IsSelected)
            {
                selected.Add(s.ID);
            }
        }

        return selected;
    }
}

I have refactored it to use generics and a dynamic type for the loop but I'm still not happy with it

     public IList<T> GetSelectedResults<T,U>(IEnumerable<U> scores)
    {
        List<T> selected = new List<T>();

        foreach (dynamic s in scores)
        {
            if (s.IsSelected)
            {
                //Need to cast the ID as <T> due to the IList limitation below
                //http://connect.microsoft.com/VisualStudio/feedback/details/534288/ilist-dynamic-cannot-call-a-method-add-without-casting
                selected.Add((T)s.ID);
            }
        }
        return selected;
    }

Any other idea?

Implement this interface in all of your Dto where it needed:

interface ICommonDtoStuff<T>
{
    bool IsSelected { get; }
    T ID { get; }
}

Then you can create a method like this:

public IList<T> GetSelectedResults<T, U>(IEnumerable<U> scores) where U : ICommonDtoStuff<T>
{
    return scores.Where(x => x.IsSelected).Select(x => x.ID).ToList();
}

As extension method:

public static class Extensions
{
    public static IList<T> GetSelectedResults<T>(this IEnumerable<ICommonDtoStuff<T>> scores)
    {
        return scores.Where(x => x.IsSelected).Select(x => x.ID).ToList();
    }
}


Return all pairs from a list of integers that contributes to a given sum Given a list of integers return all the pairs that makes up to a given
  target sum.

  input: {2, 7, 11, 15, -2}  target sum: 9

  This should return all the pairs i.e {-2,11} and {7,2}.

  If there are duplicates that form the same pair, they should not be
  returned.

Here is the code for this problem - would be great to know how this can be refactored for improvement.

   public class TwoSum {
    static class Pair {
        int x, y;

        public Pair(int x, int y) {
            this.x = x;
            this.y = y;
        }

        @Override
        public boolean equals(Object o) {
            if (o == null) {
                return false;
            }

            if (o instanceof Pair) {
                Pair other = (Pair) o;
                if (other.x == this.x && other.y == this.y) {
                    return true;
                }
            }
            return false;
        }

        @Override
        public String toString() {
            return new StringBuilder().append("{").append(this.x).append(",").append(this.y).append("}").toString();
        }
    }

    public static Set<Pair> findPairs(int inputs[], int targetSum) {
        Set<Pair> results = new HashSet<>();
        Map<Integer, Integer> map = new HashMap<>();

        for (int number : inputs) {
            int remainingSum = targetSum - number;

            if (map.containsKey(number)) {
                results.add(new Pair(number, map.get(number)));
            } else {
                map.put(remainingSum, number);
            }
        }
        return results;
    }

To invoke this code:

System.out.println(TwoSum.findPairs(new int[]{2, 7, 11, 15, -2}, 9));

This would print:

[{-2,11}, {7,2}]

Code here:

https://github.com/Ramblers-Code/CodeKata/blob/master/src/main/java/kata/array/TwoSum.java

I see that you are overriding Object.equals(Object) without overriding Object.hashCode(). By doing this, you violate the contract of Object.hashCode() (see the last paragraph in the description of Object.equals(Object)). In fact, the HashSet<Pair> created in your findPairs method might not operate properly due to this.
Since the values of x and y of a Pair instance never change, you could make x and y final, thereby making Pair an immutable class and preventing accidental tampering with the values of x and y.
I don't know if you've considered this, but your code would treat {2,3} and {3,2} as two distinct pairs. Due to how the findPairs method works, it doesn't matter, since it will never create two pairs that only differ in the order of the two numbers, but I thought I'd point it out in case you were not aware of that.
The null check in Pair.equals(Object) is redundant, since null instanceof X, with X being the name of class, will always return false.
if (o instanceof Pair) {
    Pair other = (Pair) o;
    if (other.x == this.x && other.y == this.y) {
        return true;
    }
}
return false;

can be written more concise as:

if (o instanceof Pair) {
    Pair other = (Pair) o;
    return other.x == this.x && other.y == this.y;
}
return false;

new StringBuilder().append("{")

can also be shortened:

new StringBuilder("{")

You don't actually need a Map<Integer, Integer> in findPairs. The mapping of integers to their complement serves no purpose, because the complement of an integer can be simply calculated by subtracting it from the target sum. A Set<Integer> would suffice, since you just use the map to determine whether the complement of a number has already appeared in the sequence of numbers. So findPairs can be re-written as:

public static Set<Pair> findPairs(int inputs[], int targetSum) {
    Set<Pair> results = new HashSet<>();
    Set<Integer> pastNumbers = new HashSet<>();

    for (int number : inputs) {
        int remainingSum = targetSum - number;

        if (pastNumbers.contains(remainingSum)) {
            results.add(new Pair(number, remainingSum));
        } else {
            pastNumbers.add(number);
        }
    }
    return results;
}


Replace URL parameters with Ruby I have a method to replace URL parameters in an URL. It receives url as mandatory parameter and prefix and/or hash as optional parameters. Examples:

url_replace( '/news?b=2', { b: nil } )                     # => '/news'
url_replace( '/news?b=2', { b: 3 } )                       # => '/news?b=3'
url_replace( '/news?a=b', '/bar' )                         # => '/bar?a=b'
url_replace( '/news?a=b&c=d', '/bar', c: nil )             # => '/bar?a=b'

The method:

def url_replace( target, *args )
  uri = URI.parse(URI.escape target)
  if hash = args.last.kind_of?(Hash) && args.last
    query = uri.query ? CGI.parse(uri.query) : {}
    hash.each do |k,v|
      v ? query[k.to_s] = v.to_s : query.delete(k.to_s)
    end
    uri.query = query.any? ? query.map{|k,v| "#{CGI.escape k.to_s}=#{CGI.escape Array(v).join}"}.join('&') : nil
  end
  prefix = args.first.kind_of?(String) && args.first
  uri.path = CGI.escape(prefix)  if prefix
  CGI.unescape(uri.to_s)
end

I would like some refactoring or speed optimizations.

Okay, here's the code I ended up with:

def url_replace( target, *args )
  uri = URI.parse(URI::DEFAULT_PARSER.escape target)
  uri.path = CGI.escape(args.first)  if args.first.kind_of?(String)
  if args.last.kind_of?(Hash)
    query = uri.query ? CGI.parse(uri.query) : {}
    args.last.each{ |k,v| v ? query[k.to_s] = v.to_s : query.delete(k.to_s) }
    uri.query = query.any? ? URI.encode_www_form(query) : nil
  end
  CGI.unescape(uri.to_s)
end

Some notes:

url_replace( '/news' ): Each language has its formatting rules. In Ruby almost nobody inserts spaces after and before parens.
hash = args.last.kind_of?(Hash) && args.last: I'd strongly discourage this kind of positional arguments, the method signature is severely impaired. Use an options hash instead (note that Ruby 2.0 finally provides keyword arguments).
query.delete(k.to_s). If you check my other answers you'll see I tend to favour functional programming, so I'd rewrite this using expressions instead of statements. Code is much more clean when they say what things are instead of how you change their value.
Uses of args.first in the middle of the code: Strive for declarative code, give names to things before you use them when it's not clear what they are.
I'd accept only strings as keys for the query string, or, if Activesupport is at hand, I'd call stringify_keys or Hash[h.map { |k, v| [k.to_s, v] }] at some point. This way I'd avoid mixing symbols and strings.

I'd write:

require 'uri'
require 'cgi'

def url_replace(url, options = {})
  uri = URI.parse(URI.encode(url))
  hquery = CGI::parse(uri.query)
  components = Hash[uri.component.map { |key| [key, uri.send(key)] }]
  new_hquery = hquery.merge(options[:merge_query] || {}).select { |k, v| v }
  new_query = URI.encode_www_form(new_hquery)
  new_components = {path: options[:path] || uri.path, query: new_query}
  new_uri = URI::Generic.build(components.merge(new_components))
  URI.decode(new_uri.to_s)
end

puts url_replace('/news?a=b&c=d', path: '/bar', merge_query: {"c" => nil})
#=> /bar?a=b


Setting and getting bits in C I am just starting to get my feet wet with C; and am rather enjoying myself in doing so. This is sort of a follow up to my last question here. I am posting this ordinate to hopefully getting some feedback on the logic itself, and possibly also some feedback on coding style. I do, however insist on making everything explicit. I'm not a huge fan of implicit anything... Maybe that's something I need to get over?

#include <stdio.h>

typedef enum {
    false = (int) 0, true = (int) 1
} bool;

inline void set_bit(register unsigned int *x,
        register const unsigned short int offset, register const bool value) {
    if (value)
        *x |= (1 << offset);
    else
        *x &= ~(1 << offset);
}

inline bool get_bit(register const unsigned int x,
        register const unsigned short int offset) {
    return (x & (1 << offset)) != 0;
}

int main(void) {
    unsigned int x = 0;
    int count;
    for (count = 0; count < 8; count += 2)
        set_bit(&x, count, true);
    for (count = 7; count >= 0; count--)
        printf("%d ", (int) get_bit(x, count));
    printf("\n");
    return 0;
}

I do, however insist on making everything explicit. I'm not a huge fan of implicit anything... Maybe that's something I need to get over?

No, that is very good practice and a good habit. Particularly, there are many dangerous, implicit type promotions going on in C, that explicit type casts can prevent.

However, if you want to be explicit, you must actually have quite a deep understanding of what's going on between the C lines. Until you do, you will end up causing more problems than you solve with this coding practice. Also, you do a couple of pointless things that only clutter down the code.

Here are my comments on your code:

Do not define your own bool type. Instead use the standard bool type in stdint.h.
(int) 0 This cast is pointless and adds nothing of value. Integer literals are always of the type int. Enumeration constants are also always of the type int.
register is an obsolete keyword from the dark ages, when compilers were worthless at optimizing code. Today, a modern compiler does a much better job at optimizing the code than you do, so let it do its job. Also, the register keyword causes various side-effects: for example you can't take the address of a register variable. So never use register, because there is never a reason to do so. That keyword just remains in the language for backwards compatibility.
Similarly, it is often best to let the compiler handle inlining in most cases. Because while inlining makes the code faster, it also makes the executable much larger. The compiler is likely better than you at determining when to optimize for size and when to optimize for speed.
Always use {} after each statement, even if there is only one line of code following it. Skipping {} is dangerous practice that almost certainly leads to bugs sooner or later.
You want to be explicit but miss a few cases where it actually matters.

1 << offset means that you do left shift on a signed integer. Assuming 32 bit integers, the case of 1 << 31 leads to undefined behavior. Generally, in 99.9% of the cases it doesn't make any sense whatsoever to use bitwise operators on signed integers. If you wish to write safe code, be explicit and type 1u << offset.
Because of the above, *x |= (1 << offset); implicitly stores an int into an unsigned int, which is potentially dangerous in some cases.
And also because of the above, you apply ~ to a signed int, which is always dangerous practice.
You have no function declarations, only definitions. You also don't declare the local functions as static, which is proper practice for functions that are private to the specific module.

As others have commented, naming a bool "value" and using it to store values is confusing. uint8_t would have been a more suitable type to use in this case.
In general, it is always better to use the types from stdint.h when you do bit manipulations.


Iterative Fibonacci sequence using standard library functions We're all aware of Fibonacci sequence calculators but they look like C-style code. I want to avoid that and take advantage of the standard library. This code is very short, so I'm focusing on readability and correctness.

Barring comments, will people be able to understand the code just by looking at it?
As you're probably aware, this doesn't scale well due to stack limits. Using a dynamic memory approach, i.e. auto arr = std::make_unique<type[]>(size);, this allocates A LOT of memory. Comparatively, an iterative approach doesn't need any memory allocation at all, as it doesn't store the previous results, only the last one. As a result, my approach is automatically slower. For example, this answer ran in under a second, while mine ran in 7 seconds for a size of 1000000 (and allocated 8,000,000 bytes).

What's a way to make this faster AND to cut down on the memory usage?
I've compared the output to the oeis list and it seems correct. Would this break for bigger numbers? What should I replace std::uint64_t with? While I need std::uint64_t to store the bigger numbers, it obviously doubles the amount of memory allocated.

#include <algorithm>
#include <array>
#include <functional>
#include <iostream>
#include <iterator>
#include <numeric>
#include <cstdint>

int main()
{
    std::array<std::uint64_t, 10000> arr;
    arr.fill(0);
    arr[1] = 1;
    arr[2] = 1;

    std::transform(arr.begin(), arr.end() - 2,
        arr.begin() + 1, arr.begin() + 2,
        std::plus<>());
    std::copy(arr.begin(), arr.end(),
        std::ostream_iterator<std::uint64_t>(std::cout, " "));
    return 0;
}

I think it's clear enough, but would benefit from some changes.

Minimize initialization

There really isn't any need to call fill for this, and really only the first two array items need to be initialized:

arr[0] = 0;
arr[1] = 1;

Check numeric limits

The code does not actually work beyond around the 94th term in the sequence due to mathematical overflow.  I used this code to verify that:

std::uint64_t last = 0;
unsigned i = 0;
for (const auto &n : arr) {
    if (n < last) {
        std::cout << "broke at term " << i << " because "
            << n << " < " << last << '\n';
        exit(1);
    }
    ++i;
    last = n;
}

On my machine, this produces the following output:

  broke at term 94 because 1293530146158671551 < 12200160415121876738

Prevent numeric overflow

Better than checking for overflow after it happens is to prevent the problem from occurring in the first place.  Here's a way to do that, but first we have to do some mathematics.

First, there is a closed form way to calculate the Fibonacci numbers:

$$ F(n) = \left\lfloor{\frac{\varphi^n}{\sqrt{5}} + \frac{1}{2}}\right\rfloor $$

where \$\varphi\$ is the golden ratio:

$$ \varphi = \frac{1+\sqrt{5}}{2} $$

For large numbers, we can approximate \$F(n)\$:

$$ F(n) \approx \frac{\varphi^n}{\sqrt{5}} $$

If we want to know how many decimal digits are in \$F(n)\$, we can take \$\log_{10}(F(n))\$.  So to be pedantic, we can create a new function \$D_F(n)\$ which estimates the number of decimal digits in Fibonacci term \$n\$:

$$ D_F(n) = \log_{10}\left(\frac{\varphi^n}{\sqrt{5}}\right) $$

In this case, we want it the other way around.  That is, rather that starting with \$n\$ and calculating the number of digits \$d\$, we want to start with \$d\$ and find the largest term \$n\$ that uses no more than \$d\$ digits. This just requires a little algebra to create a function \$N_F(d)\$ that gives the largest term number that requires no more than \$d\$ digits to store:

$$ N_F(d) = \frac{\log_{10}\left(\sqrt{5}\right) + d}{\log_{10}(\varphi)} $$

Now we can actually use the result in code by inserting a few lines above main into your program like so:

constexpr int fibterm(unsigned digits) {
    return (digits + log10(sqrt(5))) / log10((1+sqrt(5))/2);
}

template <typename T>
constexpr int fiblim() {
    return fibterm(std::numeric_limits<T>::digits10)+1;
}

Now your array can be completely safe:

std::array<std::uint64_t, fiblim<std::uint64_t>()> arr;

On my machine std::numeric_limits<std::uint64_t>::digits10 = 19, so fiblim<std::uint64_t>() evaluates to 93 and we can verify that indeed, \$F(92)\$ is 19 digits long and the next term, \$F(93)\$ is 20 digits.

Remember that std::numeric_limits<T>::digits10 tells us the maximum number of digits that can always be stored in a T reliably, so std::uint64_t can hold any 19-digit number. It might be able to store some larger numbers, (as indeed it can correctly calculate \$F(93)\$ in this case) but it's not guaranteed.

Also note the +1 at the end of fibterm.  This is due to the fact that we're calculating storage space required, rather than the term number.  So even if we only wanted \$F(0)\$, it requires 1 storage slot.

Consider possible enhancements

Because the numeric range of std::uint64 is limited, as we saw above, if you wish to calculate more terms, you will need to use some other type.  One could imagine using one of the many multi-precision numeric libraries out there, or, if the intent is to simply print them, you may want to calculate them in place as arrays of decimal digits.  This latter approach would mean that the computationally costly step of converting from binary to decimal could be completely eliminated, greatly speeding the execution of the program.

Be strict about language conformance

The code currently uses std::plus<>() as the last argument to std::transform but that is actually not C++11.  It's new in C++14 and it's a good and handy feature, properly used here, but it's not strictly C++11.

Omit return 0

Your compiler will automatically generate return 0 at the end of main so there's really no need to manually insert it.


Hold and validate an EAN13 code I've created a class that holds an EAN13 code. This class is an attribute of the Product class.

In the Ean13 class, the string that represents the EAN code is passed as a parameter to the constructor and the validation is performed in the constructor. I created some utility methods in that class, like sumEven, sumOdd, and isOdd, but I'm not sure if would be better to move it to a separated utility class.

@Embeddable
public class Ean13 {
    private static final RuntimeException NOT_VALID_EAN_EXCEPTION = new RuntimeException("NOT VALID EAN CODE");

    @Column(name = "ean_code", nullable = true, length = 13)
    private String code;

    public Ean13() {
    }

    public Ean13(String code) {
        validate(code);
        this.code = code;
    }

    private void validate(String code) {
        if (code == null || code.length() != 13) {
            throw NOT_VALID_EAN_EXCEPTION;
        }
        if (!CharMatcher.DIGIT.matchesAllOf(code)) {
            throw NOT_VALID_EAN_EXCEPTION;
        }
        String codeWithoutVd = code.substring(0, 12);
        int pretendVd = Integer.valueOf(code.substring(12, 13));
        int e = sumEven(codeWithoutVd);
        int o = sumOdd(codeWithoutVd);
        int me = o * 3;
        int s = me + e;
        int dv = getEanVd(s);
        if (!(pretendVd == dv)) {
            throw NOT_VALID_EAN_EXCEPTION;
        }
    }

    private int getEanVd(int s) {
        return 10 - (s % 10);
    }

    private int sumEven(String code) {
        int sum = 0;
        for (int i = 0; i < code.length(); i++) {
            if (isEven(i)) {
                sum += Character.getNumericValue(code.charAt(i));
            }
        }
        return sum;
    }

    private int sumOdd(String code) {
        int sum = 0;
        for (int i = 0; i < code.length(); i++) {
            if (!isEven(i)) {
                sum += Character.getNumericValue(code.charAt(i));
            }
        }
        return sum;
    }

    private boolean isEven(int i) {
        return i % 2 == 0;
    }

    @Override
    public String toString() {
        return code;
    }
}

@Entity
@Table(name = "tb_produtos")
public class Product implements Serializable {

    public Product() {
    }

    @Id
    @GeneratedValue
    private Integer id;

    @ManyToOne
    @JoinColumn(name = "ID_FABRICANTE")
    private Manufacturer manufacturer;

    @Column(name = "DESCRICAO")
    private String description;

    @Column(name = "URL")
    private String url;

    @Embedded
    private Ean13 ean;

    @Transient
    private Keywords keywords;

    public Integer getId() {
        return id;
    }

    public void setId(Integer id) {
        this.id = id;
    }

    public void setCode (String code){
        ean = new Ean13(code);
    }

    public Manufacturer getManufacturer() {
        return manufacturer;
    }

    public void setManufacturer(Manufacturer manufacturer) {
        this.manufacturer = manufacturer;
    }

    public String getDescription() {
        return description;
    }

    public void setDescription(String description) {
        this.description = description;
    }

    public String getUrl() {
        return url;
    }

    public void setUrl(String url) {
        this.url = url;
    }

    public Keywords getKeywords() {
        return keywords;
    }

    public void setKeywords(Keywords keywords) {
        this.keywords = keywords;
    }

}

Don't pre-generate exceptions

On the exception in your original code: It's usually not a good idea to pre-generate exception instances. The stack trace is filled in by the constructor of Throwable, not by the throw statement, as the following code snippet demonstrates:

public class StackTrace {
    private static final Throwable t = new Throwable();
    public static void main(final String... args) throws Throwable {
        throw t;
    }
}

When you pre-generate exceptions, you will not be able to tell from the stack trace which call-tree caused the exception. In your case, you would instead see the call-tree that loaded the class:

$ java StackTrace
Exception in thread "main" java.lang.Throwable
        at StackTrace.<clinit>(StackTrace.java:2)

In Java, pre-generated exceptions are usually only found in environments where the stack trace is unavailable by nature, like Java Card Classic.

Consider converting your String to int[] first.

Normally I'd stick to Strings.
However, the case of an EAN13 is special.
The nature of an EAN13 is to be a 13 digit code.
Converting your 13 character String to an int[13] first makes this aspect of the semantics of an EAN13 more obvious and simpler.

You could then use constants like private static final int CHECK_DIGIT_INDEX = 12 to make it more obvious how an EAN13 is parsed. Consequently int pretendVd = Integer.valueOf(code.substring(12, 13)); would become int pretendVd = ean13digits[CHECK_DIGIT_INDEX];

Use names which are easy to understand, i.e. communicate intent

I could not find the term "pretend" resp. pretendVd in whatever documentation about EAN13 is available to me. I would rather call it checkDigitFromCode. The variable name dv is totally surprising, as I would at least have expected it to be vd. But I think calculatedCheckDigit or checkDigitFromCalculation would be better names. Even if variables have local scope, if there are multiple variables of the same type with different semantics, I'd rather be explicit about the intent of a variable and thus give it a name which communicates the intent well.
The worst name of all is me: The code multiplies odds and then the name is me.

Method length

The method validate(String) is quite long, and it can be split into two methods as it performs validation on two levels. First it validates whether the code actually really is 13 digits. Second it validates whether the checksum digit of the 13 EAN13 digits is correct. So, two more methods could be extracted from validate(String).

Validation Framework

This is definitely far from mandatory, it's just food for thought and showing possibilities. If you use a persistency framework like a JPA implementation, or a dependency injection framework like Spring, or even both, or something similar, you might want to consider the usage of declarative validation via annotations using the javax.validation API.

Some people favor separating model and validation always. I wouldn't go that far, and I'll try to explain that with an exaggeration: Tomorrow they'll make 6 classes, one for the instance fields, one for the constants, one for the construction (factory), one for the validation, one for the instance methods and one for the static methods. Where should this stop? The SRP doesn't say that "Every software entity should do just one thing". It comes with a clause, and this clause is important: "Every software entity should do just one thing on its level of abstraction". But it also means something else: Ideally, every software entity should have just one reason to change, and a change should ideally affect only one software entity.

If the validation only changes together with the model, it might be more prudent to keep validation and model together. If the validation is to a large extent independent of the model, it might be wiser to keep them separate.

Thinking about coupling, cohesion and interfaces also helps. If the validation is so loosely coupled, i.e. decoupled from the model that it checks not a specific model but an abstract model, resp. could be reused for different similar models, it certainly should be separate. If the validation is very tightly coupled to the model, it should be more cohesive with the model. And if you insert methods in the model to interface with the validation which are only for the validation and no other user in the system, you have strong arguments for keeping the validation inside the model class.

Welcome to engineering, there's no perfect solution, it always depends... ;)

Properly declare static methods.

Your methods getEanVd(int), sumEven(String), sumOdd(String) and isEven(int) never access any instance fields and thus could all be declared static.

Because they are private methods, it could actually make you wonder why you have a parameter String code and an instance field String code with identical content, which leads to:

Consider swapping validation sequence in the constructor

I think I guess your intention of first validating, then constructing the object with the sequence validate(code); this.code = code; in the constructor. You do not want the object to be constructed before it's validated.

However, consider a few things.

The construction is not really completed until the constructor returns. Whether you throw an exception soon or late in the constructor doesn't matter from a black box perspective. It also doesn't matter from a performance perspective, as an exception is a bad case path, not a happy path. Usually the performance of bad case paths doesn't matter.

However, it matters from a whitebox correctness perspective. Someone might look at the methods sumEven(String code) and sumOdd(String code) and realize that they're checking the very same code which would eventually be stored in the field, and thus turn the signatures into sumEven() and sumOdd(). Compiles still fine, but breaks with NullPointerException because of accessing still null field code before it was assigned to a (potentially) non-null value in the constructor.

Therefore, given a choice, i.e. when the field initialization is as trivial as simply assigning parameters to fields, I'd initialize fields first, as that leads to more robust code: It will allow subsequent private instance methods to access the fields even when they are called from the constructor.

Throwing the correct exceptions

The code part if (code == null) { throw NOT_VALID_EAN_EXCEPTION; } will be very difficult for developers to debug when it happens. Developers will assume that code was a String which was not an EAN13, but they will not expect that code was null, because the generally accepted behavior and practice is that if you pass null and null is not allowed you get nothing else but a NullPointerException. Getting a different exception than NullPointerException in case of a not-allowed null is very unexpected and can make developers waste a lot of debugging time staring at their code mislead into totally false assumptions.

In general, when null is not allowed, I'd not check, I'd simply let the code crash wherever it crashes. Whether the first check in validate(String) throws NullPointerException or whether some later part like code.substring() throws NullPointerException doesn't matter, the stack trace will always show the essential part which is that it was thrown somewhere in validate() called from Ean13().


Pull Stock Data from Alpha Vantage API I am new to Python and here is my first attempt at trying to achieve anything by myself!

Essentially, I am making JSON objects with any user defined ticker through the Alpha Vantage API.  It is basically a small part of a Python wrapper for the API (I guess).  Actually, someone already made a wrapper for the API but I want to do one for myself.  Anyways, I feel like it is a bit more messy than it has to be, and I do not know how to get to the point where someone can type something like object.lastday and get the last day's trading data with the object they just made.

NOTE: not the real API key

Any advice appreciated, I am trying to get better.

import json
import urllib.request
import datetime

#mainURL that will be appended to return our requests
mainURL = "https://www.alphavantage.co/query?"
#API key that will be needed to authenticate
myKey = "F3DH8T01FS5445IB"

#For daily activity
'''
REQUEST NEEDS:
function: will ALWAYS be equal to 'TIME_SERIES_DAILY' for daily
symbol: user defined stock ticker
outputsize: default is 'compact' which only returns 100 data points,
otherwise
we can define it as 'full' to get up to 20 years worth
datatype: default is 'json', but we can also request 'csv'
'''

#loads a json object of based on what you input in dailyData(SYMBOL)
requestTypeURL = "function=TIME_SERIES_DAILY"
def dailyData(symbol, requestType=requestTypeURL, apiKey=myKey):
    symbolURL = "symbol=" + str(symbol)
    apiURL = "apikey=" + myKey
    completeURL = mainURL + requestType + '&' + symbolURL + '&' + apiURL
    with urllib.request.urlopen(completeURL) as req:
        data = json.load(req)
        return data

#making a json object for Apple and example of getting a date's activity
(test)
'''
apple = dailyData('AAPL')
print(apple["Time Series (Daily)"]["2018-03-02"])
'''

I'm going to give you a review of this as to how I'd improve this, and though it pulls some data from the other answer a little bit, it uses a bit different syntax.  Namely, I don't like using in-built f"" for formatting strings with replacements, so I use the full "".format() syntax.

Some of the changes are suggested in order to allow easy expansion of this 'API wrapper', so that you can build out new functions more easily.  Sorry about that, but I'm one of those coders who has written entire API wrappers and therefore I think in terms of "How could I expand the API" and such.

Also, my apologies for a wall-of-text, but I am overly descriptive sometimes.  I also give you the fully revised code at the bottom of this post, if you don't want to read the wall of text, however I suggest that you at least read the section headers in bold, and then the code.

Variables should be snake_case, or CAPS_CASE for 'constants' if you wish.

All variables should be in snake_case.  This technically includes the 'global' variables that are essentially constants, though I use CAPS_CASE as they are global constants that won't change (and this is technically acceptable in the PEP8 style guides).

So, all of your variables inside the functions, where applicable, are converted to snake_case, and the URL and API key variables became CAPS_CASE like so:

QUERY_URL = "https://www.alphavantage.co/query?function={REQUEST_TYPE}&apikey={KEY}&symbol={SYMBOL}"
API_KEY = "AN ACTUAL KEY"

Condense URL formatting into a single URL generation method

Your method of generating a URL is clunky.  We can glean the following information from your URL structure, though:

You need to provide the type of request, which is a specific type of data from their API.
You need to provide the symbol being looked at.
You need to provide an API key.

Since all three parts are necessary, we can use a structured string to create the URL.  We also, already, know the base URL, so we don't need to declare another variable for it.  We end up with this string for the QUERY_URL variable (again, I made this one all-caps to be a "global" or a "constant" - this is acceptable, and though it's not snake-case, it's actually acceptable):

QUERY_URL = "https://www.alphavantage.co/query?function={REQUEST_TYPE}&apikey={KEY}&symbol={SYMBOL}"

Similarly, we also have API key as API_KEY:

API_KEY = "AN ACTUAL KEY"

We'll be using this in the next session, so let's focus on that.

Make a generic 'request' function that will accept arguments and issue the request

Rather than structuring the URL and creating individual requests for each type of function you want (which will require code duplication and increase the difficulty of maintaining things down the line), we can take the entire request URL generation and the actual task of making a request and split it off into a separate function.

However, we have to be careful as to how we return the web request. The way your existing code works returns an HTTPRequest object, and not the results of the request. To get the data, you need to read() the data. The data returned from the request is in the form of bytes, and currently the json library is looking for a file pointer (for load) or a string (for loads, which is what you'll want to use). So, we've not only got to do a read() call to the request object, we also have to decode the bytes response into a string with .decode('UTF-8'), as the bytes returned from the API are UTF-8 encoded.

For reasons, I use internal functions that shouldn't be referenced from outside the Python script/library itself which start with a _, to imply that it's a protected member.  While there is no mechanism to actually do protected members, most code analysis tools will assume things with underscores are protected elements.

With all this in mind, you get this for your actual web request function, which returns the data, and gets rid of having to take the API key as an argument too:

def _request(symbol, req_type):
    with urllib.request.urlopen(QUERY_URL.format(REQUEST_TYPE=req_type, KEY=API_KEY, SYMBOL=symbol)) as req:
        data = req.read().decode("UTF-8")
    return data

The reasons for doing this become more prevalent in the next section of this review, though.

Change your dailyData function to snake_case, but take advantage of the new _request function

Now that we have the dedicated _request function, we can get rid of all of your URL construction code that you had in dailyData.  We can also reduce this function, now to be named get_daily_data, to only require you to pass in the symbol and lets URL construction take place in the actual request handling function we made.  This saves us from having to construct brand new URLs within each and every 'getter' that would get data from the API.

All this said and done, you end up with this for your get_daily_data function, which looks a bit cleaner than the original version, in my opinion:

def get_daily_data(symbol):
    return json.loads(_request(symbol, 'TIME_SERIES_DAILY'))

Now, the only thing you really need to do is make sure you're passing the proper request/function string to the _request library, and also pass the symbol.

This has an added bonus, though: because we're using _request to do the URL construction and the actual web request processing, we can easily expand this code for other functions in your API wrapper - all you need to change is the 'function' call to get different data.

Unused Libraries

You import datetime but you don't use it for anything.  So, don't include it in the import statements until you actually use it for something.

Now on to testing.  I use a slightly different form of your test code here, compressing everything into a one-liner call:

print(get_daily_data("AAPL")["Time Series (Daily)"]["2018-03-02"])

This in turn returns this data:

{'1. open': '172.8000', '2. high': '176.3000', '3. low': '172.4500', '4. close': '176.2100', '5. volume': '38453950'}

The code after all my revisions above, and after removing comments, ends up being this:

import json
import urllib.request

QUERY_URL = "https://www.alphavantage.co/query?function={REQUEST_TYPE}&apikey={KEY}&symbol={SYMBOL}"
API_KEY = "AN ACTUAL KEY"

def _request(symbol, req_type):
    with urllib.request.urlopen(QUERY_URL.format(REQUEST_TYPE=req_type, KEY=API_KEY, SYMBOL=symbol)) as req:
        data = req.read().decode("UTF-8")
    return data

def get_daily_data(symbol):
    return json.loads(_request(symbol, 'TIME_SERIES_DAILY'))

This being said, this program could be developed to take the stock ticker to check as a runtime argument, thereby permitting you to run the core code against any symbol that you define, but that's outside the scope of this review, technically.


Correct way to cover dependencies in unit tests My approach to unit testing feels very wrong. I feel like I am missing something very basic, and I'm looking for some insight please.

Let's assume that I am testing a method (I'm using NUnit) that returns an object. It might look like this:

public void TestConnection()
{
     DBConnector dbConn = new DBConnector(connString);
     Assert.IsInstanceOf(typeof(DBConnector), dbConn);
}

Let's assume too that the test passes.

Later in my test class, I create another test on an object that has a dependency on MyObject. That test method might look like this:

public void IsCallInstanceNull()
{
     DBConnector dbConn = new DBConnector(connString);
     dbConn.EstablishConnection();
     CallInstance ci = new CallInstance(dbConn);
     Assert.IsNotNull(ci);
}

I am first marking TestConnection() "public void", ensuring a basic pass, then changing the method to private so that I can use a typed object in the second method, instead of duplicating code. So my example test class becomes:

private DBConnector CreateDBConnector()
{
     var dbConn = new DBConnector(connString);
     dbConn.EstablishConnection();
     return dbConn;
}

public void IsCallInstanceNull()
{

     CallInstance ci = new CallInstance(CreateDBConnector());
     Assert.IsNotNull(ci);
}

But this feels completely wrong. No one will ever know that the constructor for DBConnector was ever tested (since not only did I change its visibility, I renamed the method!) so  how can this be refactored so that I ensure test coverage for the private method without having to leave 2 almost identical methods in the test class?

When unit testing a class you should focus only on the functionality in that class. For all other dependencies, mocking should be used. That is to say you should create "fake" implementations of the dependencies and use them to simulate the real thing. You can then run tests against the mocks to ensure that they are being used correctly according to the functionality of the class under test.

In order to do this you will have to inject all dependencies(this is pretty much always what you should be doing), likely in your constructor.

Your test example doesn't tell us too much and doesn't test anything. Instantiating a class then validating its type is not a test. Could you provide the class that you would like to test? Then we can give concrete examples.

I wrote a tutorial for using Moq to test dependencies, it can be found here:
http://stevemichael.net/how-do-i-use-moq-with-asp-net-mvc/


Implementation of Equals and GetHashCode for base class Is this a good implementation for Equals and GetHashCode for a base class in C#? If it's not good enough, can you suggest improvements for it, please?

public abstract class Entity<TKey>    //TKey = Type of the Key
{
    private string FullClassName;
    private bool KeyIsNullable;
    private Type BaseClassType;
    private bool KeyIsComplex;

    public abstract TKey Key { get; }    //Key of the object, which determine it's uniqueness

    public Entity()
    {
        FullClassName = GetType().FullName + "#";
        KeyIsNullable = typeof(TKey).IsAssignableFrom(typeof(Nullable));
        BaseClassType = typeof(Entity<TKey>);
        KeyIsComplex= !typeof(TKey).IsPrimitive;
    }

    public override bool Equals(object obj)
    {
        bool result = BaseClassType.IsAssignableFrom(obj.GetType());
        result
            = result
                && (
                        (
                            (
                                !KeyIsNullable
                                ||
                                (Key != null && ((Entity<TKey>)obj).Key != null
                            )
                        )
                        &&
                        Key.Equals(((Entity<TKey>)obj).Key )
                    )    // The key is not nullable, or (it's nullable but) both aren't null, and also equal
                    ||
                    (
                        KeyIsNullable
                        &&
                        Key == null
                        &&
                        ((Entity<TKey>)obj).Key == null
                    )
                ); // Or the key is nullable, and both are null
        return result;
    }

    public override int GetHashCode()
    {
        if ((KeyIsNullable&& Key == null) || (!KeyIsNullable&& Key .Equals(default(TKey))))
        {
            return base.GetHashCode();
        }
        string stringRepresentation = FullClassName + ((KeyIsComplex)? Key.GetHashCode().ToString() : Key.ToString());
        return stringRepresentation.GetHashCode();
    }
}

Example of a derived class:

public class Foo : Entity<int>
{
    public virtual int FooId { set; get; }
    public virtual string FooDescription { set; get; }

    public override int Key { get { return FooId; } }
}

Specific and special details to the proposal:

Any instance of a derived class is considered equal to a instance of the base class if they have the same key.
The key could be null.
If the key of the current class and the key of the comparing object are both null, the two objects are considered equal. This is because I am planning to handle just one new object at a time, and if the key is nullable, it will be null for the new object. So if I have two instances with a null key, I will consider them as the same entity.

I think you are overcomplicating things you can say simple this:

    public abstract class Entity<TKey> where TKey : struct
    {
        public abstract TKey Key { get; }

        protected bool Equals(Entity<TKey> other)
        {
            return Key.Equals(other.Key);
        }

        public override bool Equals(object obj)
        {
            if (ReferenceEquals(null, obj)) return false;
            if (ReferenceEquals(this, obj)) return true;
            var asThis = obj as Entity<TKey>;
            return asThis != null && Equals(asThis);
        }

        public override int GetHashCode()
        {
            return Key.GetHashCode();
        }
    }

No need the type magic
A key cannot be null and it should be a ValueType
An Equals and GetHashCode implementation should be symmetric and fast (your solution is slow and i'm not sure it is symmetric or not)


"Chef and Digits" Java solution I am looking for a review on the following code which is for this question. I am looking for a general review of the code. I am specifically also looking for advice on how to make this code run more efficiently.

import java.io.BufferedReader;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.InputStreamReader;
import java.util.*;
public class Main
{
    public static void main(String args[]) throws IOException
    {
        int S[] = new int[100000];
        BufferedReader in = new BufferedReader(new InputStreamReader(System.in));
        PrintWriter out = new PrintWriter(System.out);
        String s[] = in.readLine().split(" ");
        int n = Integer.parseInt(s[0]);
        int m = Integer.parseInt(s[1]);
        String St = in.readLine();
        char num[] = St.toCharArray();
        for (int j = 0; j < m; j++)
        {
            String r1 = in.readLine();
            int r = Integer.parseInt(r1);
            int sum1 = 0, sum2 = 0;
            for (int p = 0, len = num[r - 1]; p < r - 1; p++)
            {
                int diff = len - num[p];
                if (diff < 0)
                    sum1 = sum1 + diff;
                else
                    sum2 = sum2 + diff;
            }
            out.println((sum2 - sum1));
        }
        out.flush();
        out.close();
    }
}

Naming Conventions
Variables in java should be camelCase, meaning they should start with a small letter.

Meaningful names
Someone reading your code doesn't stand a chance in hell to understand what you are trying to do without actually running this code in his head...
Your class is called Main, your only method is main... at least try to tell your reader some story on what you are trying to do, especially when you post it for review!
Variable names like n, m, num, and sum add nothing to explain your code. Names like s and S are even worse.

Efficiency
It is very hard to give efficiency advice, if I can't say what exactly you are trying to achieve, but some obvious observations:

int S[] = new int[100000];

This doesn't look very efficient - you allocate all this memory up front, and I couldn't really see where you are using it...

Your is also not very defensive - you don't close your stream in case of an exception (you should use try(PrintWriter out = new PrintWriter(System.out)) block to make sure of that), and you never validate that num[r - 1] is valid, so there is a decent chance that your code will not run as expected.

Idioms
You should use the idioms relevant for the language you use, for example - sum1 += diff; instead of sum1 = sum1 + diff;
Another example - don't wrap System.out with PrintWriter simply use System.out.println(sum2 - sum1)


Blow the fuse if threshold exceeded While implementing the Retry & Breaker patterns I decided that the Breaker does more then it should so I extracted two responsibilities into their own classes. Here they are.

I stripped out the Threshold to be only data:

public class Threshold
{
    public Threshold(int count, TimeSpan interval, TimeSpan timeout)
    {
        Count = count;
        Interval = interval;
        Timeout = timeout;
    }

    public int Count { get; }

    public TimeSpan Interval { get; }

    public TimeSpan Timeout { get; }

    public override string ToString()
    {
        return $"Count = {Count} Interval = {Interval} Timeout = {Timeout}";
    }
}

The second responsibility is the object counting the events and checking if the threshold is exceeded. I call it Fuse and implemented it this way:

public class Fuse
{
    public Fuse(Threshold threshold)
    {
        Threshold = threshold;
    }

    // todo: add null check
    public IClock Clock { get; set; } = new SystemClock();

    public Threshold Threshold { get; }

    public int Count { get; private set; }

    public DateTime? Point { get; private set; }

    public bool Blown
    {
        get
        {
            return
                Clock.GetUtcNow() - Point <= Threshold.Interval &&
                Count >= Threshold.Count;
        }
    }

    public bool TimedOut
    {
        get { return (Clock.GetUtcNow() - Point) > Threshold.Timeout; }
    }

    public Fuse Increase(int value)
    {
        if (TimedOut) { Reset(); }
        Count += value;
        Point = Clock.GetUtcNow();
        return this;
    }

    public Fuse Increase()
    {
        Increase(1);
        return this;
    }

    public Fuse Reset()
    {
        Count = 0;
        Point = null;
        return this;
    }

    public override string ToString()
    {
        return $"Count = {Count} Point = \"{Point?.ToString(CultureInfo.InvariantCulture)}\" Blown = {Blown} TimedOut = {TimedOut}";
    }
}

To be able to better test it I created an abstraction for the DateTime and called it IClock:

public interface IClock
{
    DateTime GetNow();
    DateTime GetUtcNow();
}

public class SystemClock : IClock
{
    public DateTime GetNow() => DateTime.Now;
    public DateTime GetUtcNow() => DateTime.UtcNow;
}

public class TestClock : IClock
{
    public DateTime Now { get; set; }
    public DateTime UtcNow { get; set; }
    public DateTime GetNow() => Now;
    public DateTime GetUtcNow() => UtcNow;
}

Without the breaker the two new modules are now easier to test:

var threshold = new Threshold(count: 3, interval: TimeSpan.FromSeconds(5), timeout: TimeSpan.FromSeconds(10));
var fuse = new Fuse(threshold) { Clock = new TestClock { UtcNow = new DateTime(2016, 11, 12, 9, 0, 0) } };
fuse.Increase(2).ToString().Dump();
fuse.Increase().ToString().Dump();
(fuse.Clock as TestClock).UtcNow = new DateTime(2016, 11, 12, 9, 0, 0).AddSeconds(20);
fuse.ToString().Dump();
fuse.Increase().ToString().Dump();

Output:

Count = 2 Point = "11/12/2016 09:00:00" Blown = False TimedOut = False
Count = 3 Point = "11/12/2016 09:00:00" Blown = True TimedOut = False
Count = 3 Point = "11/12/2016 09:00:00" Blown = False TimedOut = True
Count = 1 Point = "11/12/2016 09:00:20" Blown = False TimedOut = False

Your interface Iclock exposes methods GetNow and GetUtcNow.
Your TestClock class implements it, but because you also want to be able to modify the values you end up by implementing the properties Now and UtcNow.

In my opinion would be a better alternative that IClock would expose a read-only properties,
and that your class would expose the public setter as well:

public interface IClock
{
    DateTime Now{ get; }
    DateTime UtcNow{ get; }
}

public class TestClock : IClock
{
    public DateTime Now { get; set; }
    public DateTime UtcNow { get; set; }
}

Your Increase method in Fuse class is hard to get grasp on.

So... I migth be wrong but you do not want to keep track of the current time when Increase method is called.
What this means is that this line Point = Clock.GetUtcNow(); would become

Point = Point ?? Clock.GetUtcNow();

Furthermore I would consider to move this to some other method.
It could be a wise choice to move it to the constructor, however if you want to have a better control about exactly which tim ite is related to you could have a start method.

public Fuse Start(){
    //if you want to you can check if Point is null and throw an exception if it's not
    Point = Clock.GetUtcNow();
}

It also seems that your Increase method sometimes decides to do his trick and call Reset method.
If I were a consumer of any API I would never guess that a method called Increase would reset something.

In my opinion one simple thing you can do it have a boolean flag that would decide if Reset should be called "automatically".

Obviously this change would be accompanied by a slightly better named method.
Unfortunately nothing better than ShortenFuse comes to mind.

public class Fuse
{
    public Fuse(Threshold threshold)
    {
        Threshold = threshold;
        AutoReset = true;
    }

    public bool AutoReset{ get; set; }

    public Fuse ShortenFuse(int value)
    {
        if (TimedOut && AutoReset) { Reset(); }
        Count += value;
        return this;
    }
    //...
}

One more thing. In your "tests" you do this:

(fuse.Clock as TestClock).UtcNow = new DateTime(2016, 11, 12, 9, 0, 0).AddSeconds(20);

Wouldn't it be much better to put the seconds on the constructor?


Python one-liner to print permutations of a string I have written a Python one-liner that permutes a given string. I wrote this just for fun using list comprehensions and I would like to get feedback from the community.

def permute(c, s=""):
    return [s] if len(s) == len(c) else [x for y in [permute(c, e) for e in [s[0:i]+c[len(s)]+s[i:] for i in xrange(len(s)+1)]] for x in y]

print permute ("omer")

Even though the code fits a single line, this is not Pythonic. Just because Python allows to put a lot of things on a single line because of list comprehensions, ternary operators and other syntactic sugar, it does not mean you have to try making your code as short as possible. Remember the Zen of Python, in particular:

  Explicit is better than implicit.

  Flat is better than nested.

  Readability counts.

  If the implementation is hard to explain, it's a bad idea.

Expand the solution into multiple lines using meaningful variable names and comments:

def permute(c, s=""):
    """Returns all possible permutations of a string c."""
    if len(s) == len(c):  # base case - returns a single variation of a string - string itself
        return [s]
    else:
        variations = [s[0:i] + c[len(s)] + s[i:] for i in range(len(s) + 1)]

        # get all permutations for each variation of a string
        permutations = [permute(c, e) for e in variations]

        # flatten a list of lists
        return [x for y in permutations for x in y]

print(permute("omer"))


Joining datatables to create a new datatable I am joining datatables to create a new datatable:

var row = from r0w1 in dt_vi.AsEnumerable()
              join r0w2 in dt_w.AsEnumerable()
              on r0w1.Field<int>("ID") equals r0w2.Field<int>("iD")
              join r0w3 in dt_re.AsEnumerable()
              on r0w1.Field<int?>("ID") equals r0w3.Field<int?>("id")
              join r0w4 in dt_def.AsEnumerable()
              on r0w1.Field<int?>("ID") equals r0w4.Field<int?>("id") into ps
              from r0w4 in ps.DefaultIfEmpty()
              select r0w1.ItemArray.Concat(r0w2.ItemArray.Concat(r0w3.ItemArray.Concat(r0w4 != null ? r0w4.ItemArray : new object[] { }))).ToArray();

foreach (object[] values in row)
    dt.Rows.Add(values);

In the above code,

foreach (object[] values in row)
    dt.Rows.Add(values);

is slow for lakhs of rows. I want to put the data of row into dt datatable. Is there any faster way of doing this?

I have managed to run some tests (I was very curious about LINQ on objects vs. SQL processing performance):

1) Setup

-- drop table t1
create table t1 (id INT PRIMARY KEY CLUSTERED, name NVARCHAR(1000))
go
-- drop table t2
create table t2 (id INT PRIMARY KEY CLUSTERED, name NVARCHAR(1000))
go
-- drop table t3
create table t3 (id INT PRIMARY KEY CLUSTERED, name NVARCHAR(1000))
go
-- drop table t4
create table t4 (id INT PRIMARY KEY CLUSTERED, name NVARCHAR(1000))
go

insert into t1 select top 200000 ROW_NUMBER() OVER (ORDER BY (SELECT 1)), CAST(text AS NVARCHAR(1000)) from sys.messages
insert into t2 select top 200000 ROW_NUMBER() OVER (ORDER BY (SELECT 1)), CAST(text AS NVARCHAR(1000)) from sys.messages where message_id > 1000
insert into t3 select top 200000 ROW_NUMBER() OVER (ORDER BY (SELECT 1)), CAST(text AS NVARCHAR(1000)) from sys.messages where message_id > 2000
insert into t4 select top 200000 ROW_NUMBER() OVER (ORDER BY (SELECT 1)), CAST(text AS NVARCHAR(1000)) from sys.messages where message_id > 3000

All tables have 2 Lakhs (200K)

Application and SQL run on the same machine (PC).

2) C# code

        public static DataTable fillDataTable(SqlConnection sqlConn, string table)
        {
            return fillDataTableByQuery(sqlConn, "SELECT * FROM " + table);
        }

        public static DataTable fillDataTableByQuery(SqlConnection sqlConn, String query)
        {
            DataTable dt = new DataTable();
            using (SqlCommand cmd = new SqlCommand(query, sqlConn))
            {
                SqlDataAdapter da = new SqlDataAdapter(cmd);

                da.Fill(dt);
            }

            return dt;
        }

            var dt = new DataTable();
            for (int i = 0; i < 4; i ++ )
            {
                dt.Columns.Add(String.Format("Id{0}", i), typeof(int));
                dt.Columns.Add(String.Format("Name{0}", i), typeof(String));
            }

            using (SqlConnection con = new SqlConnection(connectionString))
            {
                con.Open();

                var sw = new Stopwatch();
                sw.Start();

                var dt1 = fillDataTable(con, "t1");
                var dt2 = fillDataTable(con, "t2");
                var dt3 = fillDataTable(con, "t3");
                var dt4 = fillDataTable(con, "t4");
                Console.WriteLine("Loaded in " + sw.ElapsedMilliseconds);
                sw.Reset(); sw.Start();

                var row = from r0w1 in dt1.AsEnumerable().AsParallel()
                          join r0w2 in dt2.AsEnumerable().AsParallel()
                            on r0w1.Field<int>("ID") equals r0w2.Field<int>("iD")
                          join r0w3 in dt3.AsEnumerable().AsParallel()
                            on r0w1.Field<int?>("ID") equals r0w3.Field<int?>("id")
                          join r0w4 in dt4.AsEnumerable().AsParallel()
                            on r0w1.Field<int?>("ID") equals r0w4.Field<int?>("id") into ps
                            from r0w4 in ps.DefaultIfEmpty()
                            select r0w1.ItemArray.Concat(r0w2.ItemArray.Concat(r0w3.ItemArray.Concat(r0w4 != null ? r0w4.ItemArray : new object[] { }))).ToArray();

                foreach (object[] values in row)
                    dt.Rows.Add(values);

                Console.WriteLine("Processed in " + sw.ElapsedMilliseconds);
                sw.Reset(); sw.Start();

                var directDt = fillDataTableByQuery(con, @"
select t1.*, t2.*, t3.*, t4.*
from t1
    join t2 on t2.id = t1.id
    join t3 on t3.id = t2.id
    left join t4 on t4.id = t3.id
");

                Console.WriteLine("Sql process + fetch in " + sw.ElapsedMilliseconds);

3) Results

No parallel

Loaded in 4370
Processed in 3249
Sql process + fetch in 3033

With parallel

Loaded in 4240
Processed in 1904
Sql process + fetch in 3160

So, including fetch, SQL performs better in this case (IDs are all clustered indexes). Also, using AsParallel makes a difference, at least on my machine (eight-core).

[later edit] - Dictionary version

Since you have to deal with fwk. 3.5. (VS 2008) and you said that you load data once and do more processing, I think you can try out the following:

    class AggrData
    {
        public int Id1 { get; set; }
        public String Name1 { get; set; }
        public int Id2 { get; set; }
        public String Name2 { get; set; }
        public int Id3 { get; set; }
        public String Name3 { get; set; }
        public int Id4 { get; set; }
        public String Name4 { get; set; }
    }

    // value is defined as anonymous type for extensibility
    var dict1 = dt1.AsEnumerable().ToDictionary(r => r.Field<int>("ID"), r => new { Name = r.Field<String>("Name") });
    var dict2 = dt2.AsEnumerable().ToDictionary(r => r.Field<int>("ID"), r => new { Name = r.Field<String>("Name") });
    var dict3 = dt3.AsEnumerable().ToDictionary(r => r.Field<int>("ID"), r => new { Name = r.Field<String>("Name") });
    var dict4 = dt4.AsEnumerable().ToDictionary(r => r.Field<int>("ID"), r => new { Name = r.Field<String>("Name") });

    Console.WriteLine("Dictionary setup took " + sw.ElapsedMilliseconds);
    sw.Reset(); sw.Start();

    var aggrList = new List<AggrData>();
    foreach (var key in dict1.Keys)
    {
        if (!dict2.ContainsKey(key) || !dict3.ContainsKey(key))
            continue;

        var newAggr = new AggrData()
            {   Id1 = key, Name1 = dict1[key].Name,
                Id2 = key, Name2 = dict2[key].Name,
                Id3 = key, Name3 = dict3[key].Name
            };
        if (dict4.ContainsKey(key))
        {
            newAggr.Id4 = key;
            newAggr.Name3 = dict4[key].Name;
        }

        aggrList.Add(newAggr);
    }

    Console.WriteLine("Aggregate POCOs fill took " + sw.ElapsedMilliseconds);
    sw.Reset(); sw.Start();

Results:

Dictionary setup took 3654
Aggregate POCOs fill took 202

So, setup is expensive, but the actual fill is much faster, as checking and retrieving data from Dictionary is very fast - O(1).


Efficiently update string (in XML format) in MongoDb public async void Execute()
{
    var mongoClient = new MongoClient(connectionStringBuilder.ToMongoUrl());
    var db = mongoClient.GetDatabase(connectionStringBuilder.DatabaseName);
    var jobInfoDocuments = db.GetCollection<JobInfoRecord>("JobInfoRecords");

    var encryptedKeys = new[] { "AccountKey", "PrivateKey", "APIKey", "DefectiveKeyGracefulExpiration" };
    var keyPatternMatching = string.Format("({0})", string.Join("|", encryptedKeys));

    // This string is used to scan tags (in the xml string) that contain "password" in their names (case-insensitive match mode),
    // and the tags which are called exactly the same as in the encryptedKeys
    var regex = string.Format(@"(?si)<([^\s<]*password[^\s<]*|{0})>.*?</\1>", keyPatternMatching);

    var filter = Builders<JobInfoRecord>.Filter.Regex(x => x.SerializedBackgroundJobInfo, new BsonRegularExpression(regex));
    var requiredDocuments = await jobInfoDocuments.Find(filter).ToListAsync();
    foreach (var document in requiredDocuments)
    {
        const string EmptyTag = "<$1></$1>";

        var jobIdFilter = Builders<JobInfoRecord>.Filter.Eq("_id", document.JobId);
        var newInfo = Regex.Replace(document.SerializedBackgroundJobInfo, regex, EmptyTag);
        var update = Builders<JobInfoRecord>.Update.Set("SerializedBackgroundJobInfo", newInfo);

        jobInfoDocuments.UpdateOneAsync(jobIdFilter, update).Wait();
     }
}

I'm changing the values of tags in xml string that contains "password" in their names (or called exactly the same as in the encryptedKeys) to empty tag.

I have a collection with 500 000 documents, of which I am performing updates each minute (hopefully) of approx. 3000 documents.

Do I have a bugs? Maybe I can increase performance of the update operation?

I'm using Mongo driver 2.0.2

One thing that I would probably do differently is move the const string EmptyTag outside of the foreach loop, I don't think that you need to recreate this string every time you loop, move it to the beginning of the method.

Another thing that I would do, make sure that my connections are properly disposed of in all cases by using a using block where ever I could.

Mongo doesn't make use of the IDisposable interface, so you can't use the using syntax to dispose of the connection

I also removed the comment block because I could see what was being done rather easily.

Here is what I came up with,

public async void Execute()
{
    const string EmptyTag = "<$1></$1>";

    var mongoClient = new MongoClient(connectionStringBuilder.ToMongoUrl());
    var db = mongoClient.GetDatabase(connectionStringBuilder.DatabaseName);
    var jobInfoDocuments = db.GetCollection<JobInfoRecord>("JobInfoRecords");
    var encryptedKeys = new[] { "AccountKey", "PrivateKey", "APIKey", "DefectiveKeyGracefulExpiration" };
    var keyPatternMatching = string.Format("({0})", string.Join("|", encryptedKeys));
    var regex = string.Format(@"(?si)<([^\s<]*password[^\s<]*|{0})>.*?</\1>", keyPatternMatching);

    var filter = Builders<JobInfoRecord>.Filter.Regex(x => x.SerializedBackgroundJobInfo, new BsonRegularExpression(regex));
    var requiredDocuments = await jobInfoDocuments.Find(filter).ToListAsync();
    foreach (var document in requiredDocuments)
    {
        var jobIdFilter = Builders<JobInfoRecord>.Filter.Eq("_id", document.JobId);
        var newInfo = Regex.Replace(document.SerializedBackgroundJobInfo, regex, EmptyTag);
        var update = Builders<JobInfoRecord>.Update.Set("SerializedBackgroundJobInfo", newInfo);

        jobInfoDocuments.UpdateOneAsync(jobIdFilter, update).Wait();
    }
}