Turn logged data into fancy plot I'm logging data from an embedded platform over UART. This gets saved to a log file and I want to process the saved log with Excel.

I have no idea whether what I did was even remotely a good solution. My VBA experience so far consists mainly of code only used once. However, since I'll have to use this for every measurement series I conduct it's probably a good idea to clean it up. Not in the least because I can't stand ugly code, I'm about to expand on it and colleagues may want to use it in the near future as well.

Example input:

00032
00016
00016
00016
00032
00064
00080
00096
00112
00128
00160
00192

The size of the input can be thousands of values. All leading zeroes get dropped as intended. Values can be negative.

Option Explicit

Public Sub Stats()
'
' Stats Macro
' Give stats to column A and draw chart with trendline.
'
' Keyboard Shortcut: Ctrl+Shift+M
'
    Columns("A:A").Select
    Selection.FormatConditions.AddColorScale ColorScaleType:=3
    Selection.FormatConditions(Selection.FormatConditions.Count).SetFirstPriority
    Selection.FormatConditions(1).ColorScaleCriteria(1).Type = _
        xlConditionValueLowestValue
    With Selection.FormatConditions(1).ColorScaleCriteria(1).FormatColor
        .Color = 7039480
        .TintAndShade = 0
    End With
    Selection.FormatConditions(1).ColorScaleCriteria(2).Type = _
        xlConditionValuePercentile
    Selection.FormatConditions(1).ColorScaleCriteria(2).Value = 50
    With Selection.FormatConditions(1).ColorScaleCriteria(2).FormatColor
        .Color = 8711167
        .TintAndShade = 0
    End With
    Selection.FormatConditions(1).ColorScaleCriteria(3).Type = _
        xlConditionValueHighestValue
    With Selection.FormatConditions(1).ColorScaleCriteria(3).FormatColor
        .Color = 8109667
        .TintAndShade = 0
    End With

    ActiveSheet.Shapes.AddChart2(227, xlLine).Select
    ActiveChart.SetSourceData Source:=Range("Sheet1!$A:$A")
    ActiveChart.FullSeriesCollection(1).Trendlines.Add Type:=xlMovingAvg, Period _
        :=2, Forward:=0, Backward:=0, DisplayEquation:=0, DisplayRSquared:=0, _
        Name:="2 per. Mov. Avg. (Series1)"
    ActiveChart.ClearToMatchStyle
    ActiveChart.ChartStyle = 233
    ActiveChart.FullSeriesCollection(1).Trendlines(1).Select
    With Selection.Format.Line
        .Visible = msoTrue
        .ForeColor.ObjectThemeColor = msoThemeColorAccent2
        .ForeColor.TintAndShade = 0
        .ForeColor.Brightness = -0.25
        .Transparency = 0
    End With

    Range("C1").Select
    ActiveCell.FormulaR1C1 = "Average:"
    Range("D1").Select
    ActiveCell.FormulaR1C1 = "=AVERAGE(C[-3])"
    Range("C2").Select
    ActiveCell.FormulaR1C1 = "Minimum:"
    Range("D2").Select
    ActiveCell.FormulaR1C1 = "=MIN(C[-3])"
    Range("C3").Select
    ActiveCell.FormulaR1C1 = "Maximum:"
    Range("D3").Select
    ActiveCell.FormulaR1C1 = "=MAX(C[-3])"
    Range("C4").Select
    ActiveCell.FormulaR1C1 = "Median:"
    Range("D4").Select
    ActiveCell.FormulaR1C1 = "=MEDIAN(C[-3])"
End Sub

Result looks like this:

I'm mainly worried about the lack of being generic. While ActiveCell and ActiveSheet and ActiveChart work wonders, I haven't figured out how to apply the same logic everywhere. There are still values like Sheet1 which should be set automatically for the current sheet/object/etc..

Avoid using Active*

You can use Worksheets collection to access a specified Worksheet object represents a worksheet.

Worksheets("YOUR SHEET NAME")

This can be useful to do explicit job, without changing the active sheet. The same way for ActiveCell, ActiveChart and ActiveChart, you can always access the target object by the collection.

Use With rather than Selection

Aside from error-raising macro problems, a recorded macro is less efficient, because it mimics all the mouseclicks and keystrokes (every cough and camera flash) that occurred while the recording was taking place. A recorded macro clicks on every object to select it, then performs an action on the selection:

Columns("A:A").Select
Selection.FormatConditions.AddColorScale ColorScaleType:=3
Selection.FormatConditions(Selection.FormatConditions.Count).SetFirstPriority

If you have two or more property or method statements that work on the same object, wrap them in a With / End With block:

With Worksheets("Sheet1").Columns("A")
    ' read / write object's properties here
    ' e.g. add new color scale rule
    .FormatConditions.AddColorScale ColorScaleType:=3
End With

This block can increase the code efficiency and readability.

Check the existence of target object

The macro record is powerful, but it's more difficult to understand whether something exists already. For example, we may not want to apply 10 rules of color scale to the column A.

' drop color scale if exists
For Each fc In .FormatConditions
    If fc.Type = xlColorScale Then
        fc.Delete
    End If
Next

Here's just an example, there must be better logic for treating existing object.

As for your script, here's my suggest edits.

Public Sub Stats2()
'
' Stats Macro
' Give stats to column A and draw chart with trendline.
'
' Modified by Mincong HUANG
'

    ' declarations
    Dim wsName As String
    Dim chartName As String
    Dim srcRange As Range

    ' initialization
    wsName = "Sheet1"
    chartName = "Fancy Chart"
    Set srcRange = Worksheets(wsName).Columns("A")

    ' set format conditions at target column
    With srcRange

        Debug.Print .Address

        ' drop color scale if exists
        For Each fc In .FormatConditions
            If fc.Type = xlColorScale Then
                fc.Delete
            End If
        Next

        ' add new color scale
        With .FormatConditions
            ' add
            .AddColorScale ColorScaleType:=3
            With .Item(.Count)

                .SetFirstPriority

                ' advanced setting (optional)
                ' Optional because these values are the default setting
                ' but you can add other desired color

                .ColorScaleCriteria(1).Type = xlConditionValueLowestValue
                .ColorScaleCriteria(1).FormatColor.Color = 7039480
                .ColorScaleCriteria(1).FormatColor.TintAndShade = 0

                .ColorScaleCriteria(2).Type = xlConditionValuePercentile
                .ColorScaleCriteria(2).FormatColor.Color = 8711167
                .ColorScaleCriteria(2).FormatColor.TintAndShade = 0
                .ColorScaleCriteria(2).Value = 50

                .ColorScaleCriteria(3).Type = xlConditionValueHighestValue
                .ColorScaleCriteria(3).FormatColor.Color = 8109667
                .ColorScaleCriteria(3).FormatColor.TintAndShade = 0

            End With
        End With
    End With

    ' drop log chart if exists
    Debug.Print Worksheets(wsName).ChartObjects.Count & " charts"
    For Each c In Worksheets(wsName).ChartObjects
        If c.Name = chartName Then
            c.Delete
        End If
    Next

    ' add log chart
    With Worksheets(wsName).Shapes.AddChart2(227, xlLine)
        .Chart.Parent.Name = chartName
        .Chart.HasTitle = True
        .Chart.ChartTitle.Text = chartName
        .Chart.SetSourceData Source:=srcRange
        .Chart.FullSeriesCollection(1).Trendlines.Add _
            Type:=xlMovingAvg, _
            Period:=2, _
            Forward:=0, _
            Backward:=0, _
            DisplayEquation:=0, _
            DisplayRSquared:=0, _
            Name:="2 per. Mov. Avg. (Series1)"
        .Chart.ClearToMatchStyle
        .Chart.ChartStyle = 233
        With .Chart.FullSeriesCollection(1).Trendlines(1).Format.Line
            .Visible = msoTrue
            .ForeColor.ObjectThemeColor = msoThemeColorAccent2
            .ForeColor.TintAndShade = 0
            .ForeColor.Brightness = -0.25
            .Transparency = 0
        End With
    End With

    ' release memory
    Set srcRange = Nothing

End Sub

By the way, I'm using Excel 2016. So I'm not sure the code compatibility. Please tell me if there's any error.

References

Peltier Tech Blog - Quick Excel Chart VBA Examples
Peltier Tech Blog - Naming an Excel Chart
MSDN - Worksheets Object (Excel)


Extracting the first letter of a string I'm extracting the first letter (not symbol) of a string using Unicode characters, with French words/sentences in mind.

I have implemented it like this:

def lettrine(text):
    first = next((c for c in text if c.isalpha()), "")
    return first

assert lettrine(u":-)") == u""
assert lettrine(u"Éléphant") == u"É"
assert lettrine(u"\u03b1") == u"α"
assert lettrine(u":-)") == u""
assert lettrine(u"") == u""

Do you think there is a better solution? Would isalpha work just as well on both Python 2.7 and 3.5?

The only thing I can see is that you don't need to have the return on a separate line.  return next((c for c in text if c.isalpha()), "") works fine.  It works on both python 2 and python 3 from what I can see.

Also, you could use filter in this situation: return next(iter(filter(unicode.isalpha, text)), ""), although I am not sure that is any real improvement.  On python 3 this approach is a bit simpler: return next(filter(str.isalpha, text), "")


Searching for a string in a downloaded PDF This code goes to the website containing the PDF, downloads the PDF, then it converts this PDF to text. Finally, it reads this whole file (Over 5000 lines) into a list, line by line, and searches for my name in it.

import mechanize
import os
br = mechanize.Browser()
br.open("http://sttm.org/Bulletin/Bulletins2015/tabid/483/LinkClick.aspx?fileticket=WkjzL8SuMQQ%3d&tabid=483&portalid=0&mid=979")
print br.title()
pdfs = []
for i in br.links():
    if "Sunday," in i.text:
        pdfs.append("http://sttm.org/Bulletin/Buletins2015/" + i.url)
br.retrieve(pdfs[0], "file.pdf")
input = "file.pdf"
output = "out.txt"
os.system(("ps2ascii %s %s") %(input, output))
with open("out.txt") as f:
    list = f.readlines()
scheduled = False
for i in list:
    if "My Name Here" in i:
        scheduled = True;
if scheduled == True:
    print "Yes"
else:
    print "No"

It's a heck of a lot of code to complete this simple task. Also, as it is taking in 5000 lines and reading them one by one, it takes quite a long time to run this.

First of all standard library imports and third-party library imports should be kept separate:

from subprocess import PIPE, Popen
from urlparse import urljoin

import mechanize

It's better to put your code in a function so that you re-use it with different urls, search keywords etc. Further explanation in comments:

def pdf_contains(url, file_url, search_key, keyword=''):
    br = mechanize.Browser()
    br.open(url)
    print br.title()
    # As you're interested only in the first link text that contains
    # the keyword('Sunday,' in this case) we should better use next()
    # with a generator expression. `next()` will yield the first item from
    # the generator if any otherwise we'll return None
    # Also `urlparse.urljoin` can come handy in joining urls.

    pdf = next((urljoin(file_url, f.url) for f in br.links()
               if keyword in f.text), None)

    # Now instead of downloading the file using .urlretrive we
    # can simply simply get a file-like object using
    # `mechanize.urlopen` which then we can then pass to
    # subprocess's STDIN

    if pdf is not None:
        data = mechanize.urlopen(pdf)

        # Now instead of running `os.system` and storing the output
        # in a file we can use `subprocess.Popen` to store the
        # output of ps2ascii command in PIPE

        proc = Popen(['ps2ascii'], stdin=data, stdout=PIPE)

        # Now simply read the data line by line from PIPE
        # and check for the search_key, if found return instantly
        for line in iter(proc.stdout.readline, ''):
            if search_key in line:
                return True
    return False

url = ("http://sttm.org/Bulletin/Bulletins2015/tabid/483/Link"
       "Click.aspx?fileticket=WkjzL8SuMQQ%3d&tabid=483&portal"
       "id=0&mid=979", "http://sttm.org/Bulletin/Buletins2015/")

scheduled = pdf_contains(url,
                         "http://sttm.org/Bulletin/Buletins2015/",
                         "My Name Here",
                         "Sunday,")

print scheduled


Remove excessive whitespace from string Is that standards-compliant?

#include <string>
#include <algorithm>
#include <iostream>
#include <cctype>

std::string remove_excessive_ws(std::string const &str)
{
    std::string result{ str };
    auto end{ std::remove_if(std::begin(result), std::end(result),
                             [](std::string::value_type const &ch) {
                                  return std::isspace(static_cast<unsigned>(ch)) &&
                                         (std::isspace(static_cast<unsigned>((&ch)[1])) || (&ch)[1] == '\0');
                             }
              )
    };
    result.erase(end, std::end(result));
    return result;
}

int main()
{
    char const *foo{ "Hello              World!       " };
    std::cout << '\"' << remove_excessive_ws(foo) << "\"\n";
}

I'm unsure whether I should be accessing (&ch)[1]. I think it should be legal, but I am not sure. After all, std::remove_if() could copy the character and pass it to the function, so the pointer (&ch) + 1 might not be valid.

Define the problem

It's not clear from the description what's considered "excessive" whitespace.  From experimenting, it seems that the idea is to collapse multiple whitespace to a single whitespace character, except at the end of the string, where whitespace is to be completely removed.  (Whitespace at the beginning of string seems to treated the same as inner whitespace).  It's also not clear which whitespace character should be kept - the example code keeps the last one, but is that a requirement, or just an implementation choice?

Missing include

std::begin() and std::end() are declared in <iterator>.  However, there's no reason not to use the begin() and end() member functions of std::string here, as we're not operating on generic values.

When an argument is to be copied, pass by value

We don't need to copy str into result:

std::string remove_excessive_ws(std::string str)

Bug

Like all the <cctype> functions, std::isspace() requires that its argument be either EOF or representable as unsigned char.  Converting a (possibly signed) char direct to unsigned int can sign-extend to an out-of-range value.  We need to convert char to unsigned char before widening to unsigned int:

static_cast<unsigned char>(ch)

Bug

Prior to C++11, accessing the character after the end of the string is undefined behaviour (C++11 requires an extra null to follow the string data).  Thankfully, it's easy to avoid this bug by simply remembering whether the last character seen was a space.

Here's a C++11 version:

#include <algorithm>
#include <cctype>
#include <string>
#include <utility>

std::string remove_excessive_ws(std::string str)
{
    bool seen_space = false;
    auto end{ std::remove_if(str.begin(), str.end(),
                             [&seen_space](unsigned char ch) {
                                 bool is_space = std::isspace(ch);
                                 std::swap(seen_space, is_space);
                                 return seen_space && is_space;
                             })};
    // adjust end to remove end whitespace
    if (end != str.begin() && std::isspace(static_cast<unsigned char>(end[-1]))) {
        --end;
    }
    str.erase(end, str.end());
    return str;
}

We might want to move seen_space into the lambda expression in later C++ versions that allow that.

This also is more readable, as we can perform the widening to unsigned int when calling the lambda, rather than having to write a cast.

Style-wise, I'd normally prefer to name the lambda, and keep the erase-remove call on a single line so that the idiom is obvious:

// Assuming C++17 now
std::string remove_excessive_ws(std::string s)
{
    auto is_doubled_space =
        [seen_space=false](unsigned char c) mutable {
            return std::exchange(seen_space, std::isspace(c))
                && seen_space;
        };
    s.erase(std::remove_if(s.begin(), s.end(), is_doubled_space), s.end());
    // remove trailing whitespace
    if (!s.empty() && std::isspace(static_cast<unsigned char>(s.back()))) {
        s.pop_back();
    }
    // convert all whitespace into ordinary space character
    std::replace_if(s.begin(), s.end(),
                    [](unsigned char c) { return std::isspace(c); }, ' ');
    return s;
}


Insert-everywhere function This is HtDP Excercise 12.4.2:

  Develop a function insert-everywhere. It consumes a symbol and a list
  of words. The result is a list of words like its second argument, but
  with the first argument inserted between all letters and at the
  beginning and the end of all words of the second argument.

This is what I wrote:

#lang racket

;Data Definition:
;a symbol, list of symbols

;Contract:
;insert-everywhere: symbol, list of symbols -> list of list of symbols
;The function returns a list of list of symbols wherein the symbol is inserted at every position

;Example:
; (insert-everywhere 'a '(b c d)) -> '((a b c d) (b a c d) (b c a d) (b c d a))

;Definition:

(define (insert-everywhere sym los)
  (define lst-len (find-len los))
  (define (iter n)
    (cond ((= n (+ lst-len 1)) '())
          (else (cons (insert n sym los) (iter (+ n 1))))))
  (iter 0))
;The above function first finds the list length.
;Then it itereratively inserts the symbol at every position one by one, including in the end.

;Data Definition:
;list of symbols

;Contract:
;find-len : list of symbols-> number
;find the length of the list

;Example:
;(find-len (list 'b 'c 'd)) -> 3

;Definition:

(define (find-len los)
  (cond ((null? los) 0)
        (else (+ 1 (find-len (cdr los))))))

;Data Definition:
;number , symbol, list of symbols

;Contract:
;insert : position, element, list of symbols-> list of symbols
;insert the element in the given position in the list

;Example:
;(insert 2 'a '(b c d)) -> '(b c a d)

;Definition:

(define (insert pos elm los)
  (cond ((= pos 0) (cons elm los))
        (else (cons (car los) (insert (- pos 1) elm (cdr los))))))

Here is the tests I conducted:

(insert-everywhere 'a '()) ; returns '((a))
(insert-everywhere 'a '(b)) ; returns '((a b) (b a))
(insert-everywhere 'a '(b c d)) ; returns '((a b c d) (b a c d) (b c a d) (b c d a))

It works, but I think this is not the way it was supposed to be solved. In their hint, they ask to use only recursion and append, but I didn't really get it. Could someone suggest a better answer?

Check this Stack Overflow question out, which has a solution to the same problem. A simplified version would look like this

(define (insert-at pos elmt lst)
 (if (empty? lst) (list elmt)
 (if (= 1 pos)
  (cons elmt lst)
  (cons (first lst)
        (insert-at (- pos 1) elmt (rest lst))))))

(define (insert-everywhere sym los)
  (map (lambda (i)
         (insert-at i sym los)
           )
       (range 1 (+ 2 (length los)))))

There is a function written for inserting at a particular position.
Then we have our iterative function that maps over the elements in the list.

Alternatively, there is the recursive solution found in the link above. It does this by using conditions and cons (which appends to lists together). It seams a bit clunky, but map really cleans it up.


Finding cost of tile to cover W x H floor I am new to C++ and programming in general. After reading through my first book, I decided to take on some programming exercises and test what I learned. The exercise is phrased as follows:

  Find Cost of Tile to Cover W x H Floor - Calculate the total cost of tile it would take to cover a floor plan of width and height, using a cost entered by the user.

This is the code I wrote for it:

#include <iostream>

using namespace std;

int main() {

    double StartSquare, cost, width, height;    // Starting square meters and cost. Width and height of floor.

    cout << "Enter value of 1 pack of tiles: ";
    cin >> cost;

    cout << "Now enter the square metres covered by the pack: ";
    cin >> StartSquare;

    cout << "What is the width of your room? ";
    cin >> width;
    cout << "Now enter the height:\n";
    cin >> height;

    cout << "You need to cover a total of " << width * height << " square metres.\n";
    cout << "The total cost will be: " << (width * height) * cost << endl;

    return 0;
}

It does what I need it to, I just want to know whether it can be improved upon. This also brings me to question of: How do I self-review my own code? I don't want to have to constantly be asking a review of my code and rather self-assess it myself.

One of the most powerful techniques you will develop is called 'separation of concerns'. What this means when it boils down is that your program can be split into discrete logical units. Ideally one module (or in the case of c++, class) will do exactly one job without depending on any other.

This has a number of benefits in that the parts can be re-used without needing any knowledge of which parts of the program are using them.

here is your program again, split into essentially three sections, or concerns. Hopefully you will see that although there are more lines of code, the logic in the main() function is easier to follow. Also you can hopefully see that the "input_data" class is an entirely autonomous entity, which could be re-used to gather data for other parts of a bigger program:

I also fixed the small logic error in your calculation, which was easier to see when the calculations were separated from the input/output code.

#include <iostream>

using namespace std;

double area(double width, double height)
{
    return width * height;
}

double cost_to_cover(double area, double area_per_pack, double cost_per_pack)
{
    double packs_needed = area / area_per_pack;
    double cost = packs_needed * cost_per_pack;

    return cost;
}

struct input_data {
    double area_per_pack, cost_per_pack, width, height;    // Starting square meters and cost. Width and height of floor.

    void gather() {
        cout << "Enter value of 1 pack of tiles: ";
        cin >> cost_per_pack;

        cout << "Now enter the square metres covered by the pack: ";
        cin >> area_per_pack;

        cout << "What is the width of your room? ";
        cin >> width;
        cout << "Now enter the height:\n";
        cin >> height;
    }
};

int main() {

    // concern 1 - gather data
    input_data input;
    input.gather();

    // concern 2 - perform calculations
    double room_area = area(input.width, input.height);
    double cover_cost = cost_to_cover(room_area, input.area_per_pack, input.cost_per_pack);

    /// concern 3 - output the result
    cout << "You need to cover a total of " << room_area << " square metres.\n";
    cout << "The total cost will be: " << cover_cost << endl;

    return 0;
}


Prompting for a positive integer and validating user input in Java I'm just starting out with Java, and trying to make a method to get a positive integer input from the console. My currently working implementation is this:

public static Scanner sc = new Scanner(System.in);

public static int getPositiveIntInput(String message) {

    int n;

    String error_message = "Error: input must be a positive integer.";

    System.out.print(message);
    while (true) { // This is often frowned upon?

        // Check that input is integer
        while (!sc.hasNextInt()) {

            System.out.print(error_message + "\n" + message); // Gets repeated
            sc.next();

        }

        n = sc.nextInt();
        sc.nextLine();

        // Check that input is positive
        if (n > 0) {

            return n;

        } else {

            System.out.print(error_message + "\n" + message);

        }
    }

}

In order to make my code more readable and concise, I attempted to rid myself of the while (true) and several error message prints by combining the integer and positivity checks into one loop, but I'm not getting the behaviour I would expect:

public static int getPositiveIntInput(String message) {

    String error_message = "Error: input must be a positive integer.";

    System.out.print(message);

    // Check that input is integer and positive
    while (!sc.hasNextInt() || sc.nextInt() <= 0) {

        System.out.print(error_message + "\n" + message);
        sc.next();

    }

    int n = sc.nextInt();
    sc.nextLine();

    return n;

}

What would be an effective way of improving the readability and reducing repetition while achieving the same functionality as in my original implementation? Particularly regarding the while(true) loop - I've seen it's use being discouraged in other SO threads. Would this be a case where a different looping structure could/should be considered?

while (true) { // This is often frowned upon?

Frowned upon? Not that I know of.

Using a scanner for extracting numbers or tokens from a stream of input is great when the data is guaranteed to be in the expected format, and when it's ok to crash on any error.

The .nextInt and similar methods are less practical when you have to validate the input and give a chance to retry. In such cases I find it a lot more practical to read line by line, and perform the integer parsing manually, like this:

public static int getPositiveIntInput(String message) {
    System.out.print(message);

    while (true) {
        String line = sc.nextLine();
        try {
            int n = Integer.parseInt(line);
            if (n > 0) {
                return n;
            }
        } catch (NumberFormatException e) {
            // ok to ignore: let it fall through to print error message and try next line
        }
        System.out.print("Error: input must be a positive integer.\n" + message);
    }
}


Maximum product contiguous subarray I have come up with the following solution to find the maximum product for a contiguous sub-array:
def maxProduct(nums):

    max_prod = [0]*len(nums)
    min_prod = [0]*len(nums)
    for i in range(0, len(nums)):
        min_prod[i] = min(nums[i], min_prod[i-1]*nums[i], max_prod[i-1]*nums[i])
        max_prod[i] = max(nums[i], min_prod[i-1]*nums[i], max_prod[i-1]*nums[i])

    return max(max_prod)

Current solution is O(n) in space, trying to find O(1) solution for space, but I keep seem to missing it. Ideas?

Follow PEP8 style guide for variable and function naming convention. Both the variables and function names should have snake_case names.
You can use enumerate to get index and value while iterating a list.

For constant space solution, instead of using a list to keep track of current max and min products, use a variable for both positive product and negative product. If the current number is \$ 0 \$, reset them both to \$ 1 \$ (They should be initialised as \$ 1 \$ as well). At the end of for-loop (inside the loop), keep updating your current_max value to keep track of max product encountered so far.

As edge cases, you may also consider returning (or raising errors) when input was empty etc.


Splitting a text file into paragraphs and words I'm iterating through a number of text files, trying to locate all carriage-returns, and individually save the text between carriage-returns. I get the index numbers of all carriage-returns, but I haven't got the slightest about saving the text.

Basically I want to save every string between two carriage-returns into an individual variable. The next step is to save all the words within a string as an individual hash.

Here is my code so far (edited based on the help by Tin Man and Screenmutt) to get every single paragraph of a file into an array:

# script start

# outputfile
output_text = File.open("output.txt", 'w')

# directory with files
Dir.chdir("nkamp")

#count lines
lines = File.readlines("first.txt")
line_count = lines.size
text = lines.join
paragraph_count = text.split("\.\r").length
puts "#{paragraph_count} paragraphs."

#array of paragraphs
paragraphs = Array.new
contents = []
File.foreach("first.txt", "\.\r") do |paragraph|
  puts paragraph.chomp
  puts '-' * 40
  contents << paragraph.chomp
  paragraphs << paragraph.chomp
end

puts paragraphs[10]

This code gives me an array with all the paragraphs. I am using ".\r" instead of "\n\n" because the texts are copied from PDF files, and have lost the normal page layout structures.

The next step is to save an array of the words in a paragraph into the array instead of simply a string of text:

words_in_each_paragraph = Array.new

File.foreach("Ann Reg Sci (2).txt", "\.\r") do |paragraph|
    word_hash = {}
    paragraph.split(/\W+/).each_with_object(word_hash) { |w, h|
        h[w] = []
    }
    words_in_each_paragraph << word_hash
end

puts words_in_each_paragraph[8]

Which gives the following output:

{""=>[], "The"=>[], "above"=>[], "contributions"=>[], "highlight"=>[], "the"=>[], "importance"=>[], "of"=>[], "sophisticated"=>[], "modeling"=>[], "work"=>[], "for"=>[], "a"=>[], "better"=>[], "understanding"=>[], "complexity"=>[], "entrepreneurial"=>[], "space"=>[], "economy"=>[]}

Now the next step is loop through every file, and create a dynamic hash that gives me

a. a number for the article. b. a number for the paragraph. c. the list of words as seen above.

For this I need to learn how to dynamically create hashes. This is where it goes wrong:

lines = File.readlines("test.txt")
line_count = lines.size
text = lines.join
paragraph_count = text.split("\.\r").length
puts "#{paragraph_count} paragraphs."

testArray = Array.new(paragraph_count.to_i, Hash.new)
for i in 0...paragraph_count.to_i do
    testArray[i] = Hash.new
    puts "testArray #{i} has been made"
end
words_in_each_paragraph = Array.new

File.foreach("test.txt", "\.\r") do |paragraph|
    word_hash = {}
    paragraph.split(/\W+/).each_with_object(word_hash) { |w, h|
        h[w] = []
    }
    words_in_each_paragraph << word_hash
    testArray[i][:value] = word_hash
    puts testArray[i] # IT WORKS HERE #
end

puts testArray[1] # AND IT DOESN'T WORK HERE #

So there's a lot going on here -  a lot more than there needs to be. You're working way too hard for what is actually very simple.

A few notes: I can't review code that hasn't been written. You're saying you want the "article number", but I don't see code for anything like that, so I'll ignore that bit for now. Similarly, you talk about "a number of files" but all your current code has hard-coded file names (that are different from code block to code block), so I'll leave that alone too. That leaves finding paragraphs and the words in those paragraphs.

First, though, here are some of the things I noticed in your code:

Your code keeps doing the same things over and over. You open, read, and split the target file into paragraphs again and again. And in different ways.
A lot of code is never used for anything, or made entirely redundant by later code. E.g. you read all the lines in the file... and then don't use that for anything before joining all the lines into a single string again. And then you split it again, only differently. And then you read everything again a moment later...
You open the "output.txt" file for writing, but you never use it for anything. Nor do you close it again. I'd suggest just printing to stdout (i.e. puts) and then you can use I/O redirection in the shell to dump that into a file.

In all, there are problems, a lot of which, quite frankly, seem to suggest you don't understand your current code.

Here's a detailed look at your last code block

# Why use readlines when you don't need the actual lines?
lines = File.readlines("test.txt")

# Never used for anything
line_count = lines.size

# File.readlines + join is the same as just using File.read
text = lines.join

# Now you're re-splitting the text you just joined
paragraph_count = text.split("\.\r").length

# ... just so you can print this, which the next bit
# of code can do too, since it also splits the text
# into paragraphs
puts "#{paragraph_count} paragraphs."

# So all of the code above can safely be replaced with:
# Nothing (just delete it)

# When you use Array.new with a length and an object,
# that object is *repeated* - not cloned - across the
# array. So each index in the array points to the
# exact same object.
# This would likely lead to a bug (except that your
# next bit of code makes all of this redundant)
# Also, Ruby convention is to use underscores, so
# "test_array", not "testArray". Of course, that name
# is bad to begin with because it doesn't give you any
# hints about what it contains.
testArray = Array.new(paragraph_count.to_i, Hash.new)

# You never need to use a basic for loop in Ruby, ever.
# Want you want is Enumerable#map, or - better yet -
# use a block initializer in the first place to populate
# the array
for i in 0...paragraph_count.to_i do
    # So here, you overwrite each index, making the
    # the above array initialization pointless
    testArray[i] = Hash.new
end

# Elsewhere in your post, you just used `[]` to create
# a new, empty array. Why not do that here? It's the
# most common way of doing it.
words_in_each_paragraph = Array.new

# And now you open the file yet again, and split it
# into paragraphs. Didn't we just do all this?
File.foreach("test.txt", "\.\r") do |paragraph|

    # So you make a hash, and make each word a key in
    # that hash. Presumably because you only want unique
    # words. But that's what Array#uniq is for
    word_hash = {}
    paragraph.split(/\W+/).each_with_object(word_hash) { |w, h|
        h[w] = []
    }

    # Why do this? You can transform the structure for
    # testArray into words_in_each_paragraph, and back
    # again, as needed, rather than build two pretty
    # similar arrays.
    words_in_each_paragraph << word_hash
    testArray[i][:value] = word_hash
end

So yeah, there are issues.

Here's my take on practically all of your code: How to get an array, where each item is itself an array of unique words in the paragraph

# read file, split into paragraphs, and map each paragraph
# into its unique, constituent words
paragraphs = File.read("test.txt").split(/\s*?\r\s*/).map do |paragraph|
  paragraph.scan(/[[:alnum:]]+/).uniq
end

Done. That's all of it in 3 lines.

And it's more careful with its regexp patterns: It doesn't split at anything plus \r, and it'll include non-"word" characters in words, so it won't split non-English names for instance.


"Little Deepu and Array" challenge I'm trying to solve this problem. I changed Scanner and used BufferedReader but I'm still getting TLE with the given code. Can I have a code review to help determine where else performance can be optimized?

import java.util.ArrayList;
import java.util.List;
import java.io.BufferedReader;
import java.io.InputStreamReader;

class LittleDeepuAndArray {
    public static void main(String args[] ) throws Exception {

        BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        String line = br.readLine();
        long N = Long.parseLong(line);
        List<Long> list = new ArrayList<Long>();
        String line5 = br.readLine();
        String[] numbers = line5.split(" ");
        for(int z=0;z<N;z++) list.add(Long.parseLong(numbers[z]));
        String line2 = br.readLine();
        long M = Long.parseLong(line2);
        long X=0;
        int k=0,m=0;
        long zz=0;
        long size=list.size();
        for(k=0;k<M;k++) {
            X = Long.parseLong(br.readLine());
            for (m=0;m<size;m++) {
                zz=list.get(m);
                if(X<zz) list.set(m,zz-1);
            }
        }
        for (m=0;m<size;m++) {
            if(m==size-1) System.out.print(list.get(m));
            else System.out.print(list.get(m) + " ");
        }
    }
}

Your major performance issue is almost certainly the way you perform your output. This is a common problem in how online judges evaluate performance.... they assume output time is part of the algorithmic solve-time.
Output
When programs output data to the console it is seldom a performance critical aspect of code. So, why should that code be lightning-fast when the console redraws are slow. In other words, Java's System.out.print* methods are not designed to be as fast as some other languages. Specifically, the print*() methods are synchronized so that only one thread can print at a time. They also incur a fair amount of overhead to own, manage, and release the system resources attached to the console. Each time you call print*(...) you repeat a lot of this work.
The bottom line is that you should call these methods just once.
    StringBuilder sb = new StringBuilder();
    for (m=0; m < size; m++) {
        if(m > 0) {
            sb.append(" ");
        }
        sb.append(list.get(m));
    }
    System.out.println(sb);

Put all the data in to a StringBuilder, and then print it once.
Other things
In addition, you code would benefit from using a number of more convenient mechanisms.... like whitespace and braces.
Your code is suffocating, it has no breathing room. Consider:

    String[] numbers = line5.split(" ");
    for(int z=0;z<N;z++) list.add(Long.parseLong(numbers[z]));
    String line2 = br.readLine();

which should be:
    String[] numbers = line5.split(" ");
    for(int z=0; z < N; z++) {
        list.add(Long.parseLong(numbers[z]));
    }
    String line2 = br.readLine();

Similarly, in other places you have no whitespace or braces on conde blocks or around operators.
Additionally, your variable names are irritating, and inconvenient. You have two variables called "m", both M and m.
Finally, if you put your code in to more methods, it will give the Java compiler a chance to optimize the code when it compiles it. Java's basic using of performance compilation is a "method", since you only have one method, and since it is only called once (the main method), your logic is never compiled and optimized properly.


Parsing XML to get all elem.tag: elem.text pairs I am parsing ICD-10 codes and have achieved a result that satisfies my base case, but I'm worried about how fragile my method is:

import xml.etree.ElementTree as ET

# A sample of the larger XML file I'm parsing
data = '''<diag>
<name>A00</name>
<desc>Cholera</desc>
<diag>
  <name>A00.0</name>
  <desc>Cholera due to Vibrio cholerae 01, biovar cholerae</desc>
  <inclusionTerm>
    <note>Classical cholera</note>
  </inclusionTerm>
</diag>
<diag>
  <name>A00.1</name>
  <desc>Cholera due to Vibrio cholerae 01, biovar eltor</desc>
  <inclusionTerm>
    <note>Cholera eltor</note>
  </inclusionTerm>
</diag>
<diag>
  <name>A00.9</name>
  <desc>Cholera, unspecified</desc>
</diag>
</diag>'''

# Create the tree
tree = ET.ElementTree(ET.fromstring(data))

# the `iter` method returns all tags in a given tree
# I'm just grouping the tags and texts here
def get_all_elements(tree):
    return [(elem.tag, elem.text) for elem in tree.iter()]

# This will return the desired elements from my tree
def parse_elements(tree):

    # First, get all of the elements in the tree
    elements = get_all_elements(tree)

    to_return = {}

    # This is what I think is too fragile. I'm basically looking
    # ahead for each element to see whether the next two elements
    # match the diag -> name -> desc sequence. But this indexing seems
    # to be too fragile at scale.
    for idx, elem in enumerate(elements):
        if 'diag' in elem and 'name' in elements[idx+1] and 'desc' in elements[idx+2]:
            name = elements[idx+1]
            desc = elements[idx+2]
            to_return[name[1]] = desc[1]
    return to_return

res = parse_elements(tree)

Let's see what's in res:

for k,v in res.items():
    print("name (code): ", k, "\n  desc: ", v)

name (code):  A00.9
  desc:  Cholera, unspecified
name (code):  A00.0
  desc:  Cholera due to Vibrio cholerae 01, biovar cholerae
name (code):  A00
  desc:  Cholera
name (code):  A00.1
  desc:  Cholera due to Vibrio cholerae 01, biovar eltor

So, I achieve my desired output, but I keep thinking there much be a better way to parse this XML. Unfortunately there's no <diag><subdiag></subdiag></diag>-type of hierarchy. Even subdiagnoses are labeled with the <diag> tag. I suppose the above is my attempt at recursion -- although I have found true recursion tough with the current tag-naming. At the end of the day, I just need the name: desc pairs.

Edit:

There's also this approach:

names = [x.text for x in ET.fromstring(data).findall('.//name')]
descs = [x.text for x in ET.fromstring(data).findall('.//desc')]

res = zip(names, descs)

But I don't think this approach scales well, as the number of elements in names and descs differed by about 1000 when I tested this on the larger XML file. There were mismatches between codes and descriptions when I validated on actual codes.

XPath support fetching parent element https://docs.python.org/2/library/xml.etree.elementtree.html#supported-xpath-syntax. Let's try to find name element and get parent of this element:

>>> [x for x in tree.findall('.//name/..')]
[<Element 'diag' at 0x7f9c8cece278>,
 <Element 'diag' at 0x7f9c85a84908>,
 <Element 'diag' at 0x7f9c85a93f98>,
 <Element 'diag' at 0x7f9c85a8f188>]

When we have parent element, we can get name and desc elements:

>>> [(x.find('name'), x.find('desc')) for x in tree.findall('.//name/..')]
[(<Element 'name' at 0x7f9c875ba7c8>, <Element 'desc' at 0x7f9c8cedfe58>),
 (<Element 'name' at 0x7f9c85a84958>, <Element 'desc' at 0x7f9c85a84f98>),
 (<Element 'name' at 0x7f9c85a8f048>, <Element 'desc' at 0x7f9c85a8f098>),
 (<Element 'name' at 0x7f9c85a8f1d8>, <Element 'desc' at 0x7f9c85a8f228>)]

And finally:

>>> [(x.find('name').text, x.find('desc').text) for x in tree.findall('.//name/..')]
[('A00', 'Cholera'),
 ('A00.0', 'Cholera due to Vibrio cholerae 01, biovar cholerae'),
 ('A00.1', 'Cholera due to Vibrio cholerae 01, biovar eltor'),
 ('A00.9', 'Cholera, unspecified')]


Search a string for barcodes but ignore certain characters I had to write a string search function that would search for barcodes, and be forgiving of minor formatting differences. For example, the barcode may be entered as YF 1942-AB in the customer's database, but a barcode scanner may read the barcode as YF1942AB.

The goal is to get the correct substring indices (range) for highlighting the text on an element, such as:

let text = "Serial: YF 1942-B (Scanned)"
let range = text.rangeOfStringWithIgnoredCharacterSet("YF1942B")
print(range) // (8, 16)

The approach I took was to do a regular string search but just ignore certain characters, and "sanitize" the search string before doing the search in earnest.

import Foundation

extension String {

    subscript (i: Int) -> Character {
        return self[self.startIndex.advancedBy(i)]
    }

    subscript (i: Int) -> String {
        return String(self[i] as Character)
    }

    subscript (r: Range<Int>) -> String {
        let start = startIndex.advancedBy(r.startIndex)
        let end = start.advancedBy(r.endIndex - r.startIndex)
        return self[Range(start ..< end)]
    }

    func rangeOfStringWithIgnoredCharacterSet(search: String, ignoredCharacterSet: NSCharacterSet = NSCharacterSet.alphanumericCharacterSet().invertedSet, options: NSStringCompareOptions = .CaseInsensitiveSearch) -> Range<String.Index>? {
        let newSearch = search.componentsSeparatedByCharactersInSet(ignoredCharacterSet).joinWithSeparator("")
        if newSearch.isEmpty {
            return nil
        }
        var i = 0
        while i < self.characters.count {
            if newSearch.characters.count > self.characters.count - i {
                break
            }
            var startIndex = -1
            var endIndex = 0
            var j = 0
            while j < newSearch.characters.count {
                let k = i + j
                if k >= self.characters.count {
                    break
                }
                let c: String = self[k]
                if c.rangeOfCharacterFromSet(ignoredCharacterSet) != nil {
                    i += 1
                } else if c.compare(newSearch[j], options: options) == .OrderedSame {
                    if startIndex == -1 {
                        startIndex = k
                    }
                    endIndex = k
                    j += 1
                } else {
                    break
                }
            }
            if j == newSearch.characters.count && startIndex >= 0 {
                return self.startIndex.advancedBy(startIndex)...self.startIndex.advancedBy(endIndex)
            }
            i += 1
        }
        return nil
    }
}

It all works properly, but it definitely feels a bit bolted together, since I'm using Int indices as well as dealing with String.Index types. I'd prefer to use String.Index exclusively, if possible, but Swift doesn't appear to have the equivalent of std::iterator<T>::end from the C++ STL, so I'm not sure if this is even possible.

edit: The first time I read your code I mis-interpreted what was happening because of the signature of the function, and I suggested an approach that you were already using ¯_(ツ)_/¯

I like managing the loop using String.Index instead of Int because it saves a lot of casting you'd need to do otherwise (this is all in Swift 3).

let text = "hello"
var position = text.startIndex
while position < text.endIndex {
    let character = text[position] // get substring at range
    print(character)
    position = text.index(after: position) // increment index by 1
}
// -> prints
// h
// e
// l
// l
// o

Here's my approach that I hope gives you some inspiration or ideas!

edit: updated to loop over unicode scalars so it's easier to use exclusions.contains(currentCharacter)

extension String {

    func range(of target: String, ignoring exclusions: CharacterSet) -> Range<String.Index>? {

        var startPosition = unicodeScalars.startIndex

        let limit = unicodeScalars.index(unicodeScalars.endIndex, offsetBy: -target.unicodeScalars.count)

        while startPosition < limit {

            var targetPosition = target.unicodeScalars.startIndex
            var currentPosition = startPosition

            while currentPosition < unicodeScalars.endIndex  {

                // get characters at current & target positions then increment the current position
                let currentCharacter = unicodeScalars[currentPosition]
                let targetCharacter = target.unicodeScalars[targetPosition]

                print("target", targetCharacter, "current", currentCharacter)

                currentPosition = unicodeScalars.index(after: currentPosition)

                if currentCharacter == targetCharacter {
                    // if the current character matches the target character:
                    // - increment the target posistion
                    // - return the range if there are no more characters to match in the target
                    targetPosition = target.unicodeScalars.index(after: targetPosition)

                    if targetPosition == target.unicodeScalars.endIndex,
                        let start = startPosition.samePosition(in: self),
                        let end = currentPosition.samePosition(in: self) {

                        return start..<end
                    }
                }
                else if targetPosition == target.unicodeScalars.startIndex || !exclusions.contains(currentCharacter) {
                    // if:
                    // - we're still at the start of the target; or
                    // - the current chracter isn't one of our valid exclusion characters
                    // then:
                    // - advance the start position and break out of the loop
                    startPosition = unicodeScalars.index(after: currentPosition)
                    break
                }
            }
        }
        return nil
    }
}

Testing

let messy = "Serial: YF 1942-B (Scanned)"

if let range = messy.range(of: "YF1942B", ignoring: CharacterSet.alphanumerics.inverted) {
    // -> Range<String.Index>(8, 17)

    messy.substring(with: range)
    // -> "YF 1942-B"
}


Tic-Tac-Toe game in Python-3.x I would really appreciate a general review of my Tic Tac Toe game in Python as I have spent a lot of time on it. Any improvements or suggestions?

def print_board(board):
    print "The board look like this: \n"
    for i in range(3):
        print " ",
        for j in range(3):
            if board[i*3+j] == 1:
                print 'X',
            elif board[i*3+j] == 0:
                print 'O',
            elif board[i*3+j] != -1:
                print board[i*3+j]-1,
            else:
                print ' ',

            if j != 2:
                print " | ",
        print

        if i != 2:
            print "-----------------"
        else:
            print

def print_instruction():
    print "Please use the following cell numbers to make your move"
    print_board([2,3,4,5,6,7,8,9,10])

def get_input(turn):

    valid = False
    while not valid:
        try:
            user = raw_input("Where would you like to place " + turn + " (1-9)? ")
            user = int(user)
            if user >= 1 and user <= 9:
                return user-1
            else:
                print "That is not a valid move! Please try again.\n"
                print_instruction()
        except Exception as e:
            print user + " is not a valid move! Please try again.\n"

def check_win(board):
    win_cond = ((1,2,3),(4,5,6),(7,8,9),(1,4,7),(2,5,8),(3,6,9),(1,5,9),(3,5,7))
    for each in win_cond:
        try:
            if board[each[0]-1] == board[each[1]-1] and board[each[1]-1] == board[each[2]-1]:
                return board[each[0]-1]
        except:
            pass
    return -1

def quit_game(board,msg):
    print_board(board)
    print msg
    quit()

def main():

    # setup game
    # alternate turns
    # check if win or end
    # quit and show the board

    print_instruction()

    board = []
    for i in range(9):
        board.append(-1)

    win = False
    move = 0
    while not win:

        # print board
        print_board(board)
        print "Turn number " + str(move+1)
        if move % 2 == 0:
            turn = 'X'
        else:
            turn = 'O'

        # get user input
        user = get_input(turn)
        while board[user] != -1:
            print "Invalid move! Cell already taken. Please try again.\n"
            user = get_input(turn)
        board[user] = 1 if turn == 'X' else 0

        # advance move and check for end game
        move += 1
        if move > 4:
            winner = check_win(board)
            if winner != -1:
                out = "The winner is "
                out += "X" if winner == 1 else "O"
                out += " :)"
                quit_game(board,out)
            elif move == 9:
                quit_game(board,"No winner :(")

if __name__ == "__main__":
    main()

A few quick comments, in no particular order:

Python 3 uses print as a function, not a statement

You’ve tagged the question with [python-3.x], but the code uses print statements, not functions. If I try to run the code with Python 3, I immediately get an error:

  File "tictactoe.py", line 2
    print "The board look like this: \n"
                                       ^
SyntaxError: Missing parentheses in call to 'print'

Henceforth I’ll assume that you’re actually using Python 2.x, and the question was just mistagged.

Don’t catch overly broad exceptions.

In your get_input() function, you have except Exception as e, to catch the case where the user enters something that isn’t an integer. You can improve this in two ways:

Only catch the exception you‘re expecting to throw. If you catch plain Exception, you risk losing information about unexpected errors.

In this case, you might get a ValueError if the user gives a string which can’t be coerced to an integer, so catch ValueError’s instead.
Limit the amount of code in the try block. The only line you expect to throw an exception is user = int(user), but you’ll catch exceptions thrown in any of the surrounding lines. Better to only include that line in the try block – again, you won’t lose information about unexpected errors.

Here’s how you can rewrite the body of the get_input() function accordingly:

user = raw_input("Where would you like to place " + turn + " (1-9)?
try:
    user = int(user)
except ValueError:
    print user + " is not a valid move! Please try again.\n"
    continue
if 1 <= user <= 9:
    return user-1
else:
    print "That is not a valid move! Please try again.\n"
    print_instruction()

That said, I was unable to find any invalid input that caused the script to crash – kudos.

Looking further, I see something worse in check_win():

try:
    if board[each[0]-1] == board[each[1]-1] and board[each[1]-1] == board[each[2]-1]:
        return board[each[0]-1]
except:
    pass

You should never do except: pass. This will (silently) swallow every form of exception – including SystemExit and KeyboardInterrupt, which you almost certainly don’t want to catch. In this case, you probably want to catch KeyError instead.

Get rid of the valid variable in get_input()

Its value is never actually used – it just serves to keep the loop going perpetually until the function returns. Better to use while True than track an unused variable.

Likewise for win in the main function.

Add comments and docstrings

There’s very little in the way of commentary in this code. That makes it hard to follow, and it will make things difficult when you come to maintain it in future.

Comments will help understand why you’ve written the code this way – for example, in get_input(), why do we decrement 1 from the value provided by the user? And docstrings are important if you want your code to be reusable.


Simple DateTime abstraction Some of my tests require that I need to test the date time results (like time stamps etc.). In order to be able to test the date time string I created a simple DateTime abstraction that I'm going to use in other projects later. It should replace direct calls to DateTime.(Utc)Now. Or should I rather call it TestValue instead of ConstValue?

public abstract class DateTimeProvider
{
    public abstract DateTime Value { get; }

    public DateTime? ConstValue { get; set; }

    public static implicit operator DateTime(DateTimeProvider dateTimeProvider)
    {
        return dateTimeProvider.Value;
    }
}

public class NowDateTimeProvider : DateTimeProvider
{
    public override DateTime Value
    {
        get { return ConstValue ?? DateTime.Now; }
    }
}

public class UtcNowDateTimeProvider : DateTimeProvider
{
    public override DateTime Value
    {
        get { return ConstValue ?? DateTime.UtcNow; }
    }
}

The idea of abstracting DateTime.Now and DateTime.UtcNow for testability is definitely a good one. There are some improvements I'd suggest:

Naming

FooProvider is a name I usually pick when I absolutely can't think of a better name for something that provides Foo. In this case, a much more natural name than DateTimeProvider would be Clock.

I also don't really like the name ConstValue. For one thing it's an unnecessary shortening of ConstantValue. But probably Override would be more descriptive.

Implicit Operator

I tend to lean towards being pretty permissive with the use of the implicit operator. It's often a good way to be able to write more expressive code without cluttering things up with constructor calls.

But in this case I think the benefit is very marginal. The difference in readability between calling e.g. AddTimestamp(clock) and AddTimeStamp(clock.Value) is very minor, and the latter is actually more expressive of that fact that you're stamping it with the time at the point that the AddTimeStamp method was called (rather than at some arbitrary point during its procedure).

Design

Splitting Now and UtcNow into different classes seems weird, for a couple of reasons:

It implies that you need both in your system, but never in the same place. If you did need them in the same place, you'd need to handle two different clocks, which would be cumbersome.
Similarly, it's quite possible you'll have a class which should make its own decision about whether to use Now or UtcNow. It may be a natural part of a class's responsibility to make that decision, not something that should be chosen in bootstrapping. In that case, you'd need to give the class some kind of DateTimeProviderProvider so it could get the right one!

So with that in mind I'd suggest an intermediary step of changing to:

public abstract class Clock
{
    public abstract DateTime Now { get; }

    public DateTime? NowOverride { get; set; }

    public abstract DateTime UtcNow { get; }

    public DateTime? UtcNowOverride { get; set; }
}

Override logic

Admittedly, the above actually looks worse than what we started with, but there's one more problem to tackle: the use of override-type logic. The issues with this:

It's hard to test. Your NowDateTimeProvider exists so tests don't have to deal with the real current date time. But that class itself can't be tested without dealing with the real time. Although it's a simple class, it still has logic, which means not being able to test it is a problem.
Your production code has to deal with testing concerns. Every clock you write can have the time overridden, even though there's no scenario in real code where this should be allowed. It's unsafe, and even if you're careful, it degrades the expressiveness of your code to have "Don't call me!" methods.
It's more code. Instead of a simple Now, you need a Now, a NowOverride, and logic to glue them together.

Fortunately, none of these are hard to fix:

public abstract class Clock
{
    public abstract DateTime Now { get; }

    public abstract DateTime UtcNow { get; }
}

public class RealClock : Clock
{
    public override DateTime Now { get { return DateTime.Now; } }

    public override DateTime UtcNow { get { return DateTime.UtcNow; } }
}

public class TestClock : Clock
{
    public override DateTime Now { get; set; }

    public override DateTime UtcNow { get; set; }
}

And finally...

From that last version, you can see that once we've made these changes, there's no need to have an abstract class anymore. We're left with just signatures, so we can change to an interface. This also means we can consider not writing TestClock at all and using a mocking library instead.


Flipping coins performance This is the Flipping coins problem on CodeChef:

  There are \$N\$ coins kept on the table, numbered from \$0\$ to \$N - 1\$. Initially, each coin is kept tails up. You have to perform two types of operations:

  Flip all coins numbered between \$A\$ and \$B\$ inclusive. This is represented by the command "0 A B"
  Answer how many coins numbered between \$A\$ and \$B\$ inclusive are heads up. This is represented by the command "1 A B".

  Input:

  The first line contains two integers, \$N\$ and \$Q\$. Each of the next \$Q\$ lines are either of the form "0 A B" or "1 A B" as mentioned above.

  Output:

  Output 1 line for each of the queries of the form "1 A B" containing the required answer for the corresponding query.

Out of all the submissions in Python 3.5, none was able to solve it in the provided time limit (probably 0.3s, it is not specified exactly). How can I optimize my code?

n,q = map(int,input().split())
c = [0]*n
for i in range(q):
    j,k,l = list(map(int,input().split()))
    co = 0
    if j == 1:
        #do Something
        for m in range(k,l+1):
            if c[m] == 1:
                co += 1
        print(co)
    if j == 0:
        #do something
        for m in range(k,l+1):
            if c[m] == 1:
                c[m] = 0
            else:
                c[m] = 1

The code you have posted is straightforward and basic. Because of this, I'm going to assume you are not super-experienced with Python. I'll make some direct suggestions to go from your current code to a faster version of your code, but I don't expect you'll be able to make the time limit with this structure. See @vnp's answer for how to do that.

You wrote:

n,q = map(int,input().split())
c = [0]*n
for i in range(q):
    j,k,l = list(map(int,input().split()))
    co = 0
    if j == 1:
        #do Something
        for m in range(k,l+1):
            if c[m] == 1:
                co += 1
        print(co)
    if j == 0:
        #do something
        for m in range(k,l+1):
            if c[m] == 1:
                c[m] = 0
            else:
                c[m] = 1

When I read the assignment, this is what I saw:

  There are N coins kept on the table, numbered from 0 to N - 1.
  Initially, each coin is kept tails up. You have to perform two types
  of operations:

  1) Flip all coins numbered between A and B inclusive. This is
  represented by the command "0 A B" ...

Yet, when I look at your code, I see this:

for i in range(q):
    j,k,l = list(map(int,input().split()))

You aren't mapping the problem space into your code. This makes it harder for you- and me, and everyone else that sees it- to reason about your code.

Let me suggest right up front that you adopt names that reflect the problem statement whereever possible. Note that in cases where you get clever and use a different algorithm, the names are entirely up to you. In that case, they should not conflict with the problem statement:

N, num_opns = list(map(int, input().split()))

for i in range(num_opns):
    operation, A, B = list(map(int, input().split()))

See the difference? N is N. There are operations that can be 0 or 1. Guess which is the operation? And there are parameters A and B, clearly marked. I haven't changed how it functions, but someone reading the code can now refer to the instructions and see what's what.

Adding Faster

Next, let's look at your two operations. Here's your first one, counting the 1 values:

for m in range(k,l+1):
    if c[m] == 1:
        co += 1

Your code looks at every value, and if it's 1, adds 1 to the count, and if it's not 1, adds nothing to the count.

Now, think like a programmer! Ask, "how can I get rid of any of that code?" In this case, what happens when c[m] is not equal to 1? Why, then, it's equal to 0. And what happens when you add 0 to any number? Nothing - the number doesn't change. So you could change that code to:

for m in range(k,l+1):
    co += c[m]

And get the same result!

Going further, it turns out that Python has a built-in sum that adds up a sequence of values! You can switch from adding these numbers in Python to having Python's interpreter add the numbers in C. That should speed things up nicely:

co = sum(c[k:l+1])

There's two things going on in there. The first, c[k:l+1] is called a slicing expression. It creates a "sub-list" of the c list, with just the values in question. The second is the use of the sum() built-in.

Switching Faster

Your second operation just changes values:

for m in range(k,l+1):
    if c[m] == 1:
        c[m] = 0
    else:
        c[m] = 1

There are a couple of ways to speed this up, and they all involve doing some kind of single operation instead of an if statement. For example, you could perform a bitwise exclusive-or of the value with 1. Bitwise xor is also bitwise (no borrow) subtraction modulo 2, so when you xor things 0 becomes 1, and 1 becomes 0.

for m in range(k,l+1):
    c[m] ^= 1

Alternatively, you could store the values in a tuple and map the old value to the new value:

new_values = (1, 0)
for m in range(k,l+1):
    c[m] = new_values[c[m]]

Finally, I'll point you at the timeit module, which is the standard Python way of measuring speed. If you are trying to beat a time limit in any challenge, make sure you use timeit so you can keep track of what effect every change you make has on performance.


Importing database of 4 million rows into Pandas DataFrame I am using the following code to import database table into a DataFrame:

def import_db_table(chunk_size, offset):
    dfs_ct = []
    j = 0
    start = dt.datetime.now()
    df = pd.DataFrame()

    while True:
        sql_ct = "SELECT * FROM my_table limit %d offset %d" % (chunk_size, offset)
        dfs_ct.append(psql.read_sql_query(sql_ct, connection))
        offset += chunk_size

        if len(dfs_ct[-1]) < chunk_size:
            break

        df = pd.concat(dfs_ct)

        # Convert columns to datetime
        columns = ['col1', 'col2', 'col3','col4', 'col5', 'col6',
                   'col7', 'col8', 'col9', 'col10', 'col11', 'col12',
                   'col13', 'col14', 'col15']

        for column in columns:
            df[column] = pd.to_datetime(df[column], errors='coerce')

        # Remove the uninteresting columns
        columns_remove = ['col42', 'col43', 'col67','col52', 'col39', 'col48','col49', 'col50', 'col60', 'col61', 'col62', 'col63', 'col64','col75', 'col80']

        for c in df.columns:
            if c not in columns_remove:
                df = df.drop(c, axis=1)

        j+=1
        print('{} seconds: completed {} rows'.format((dt.datetime.now() - start).seconds, j*chunk_size))

    return df

I am calling it with:

df = import_db_table(100000, 0)

This seems to be very slow - it starts with importing 100000 rows in 7 seconds but later after 1 million rows the number of seconds needed grows to 40-50 and more. Could this be improved somehow? I am using PostgreSQL, Python 3.5.

7 seconds: completed 100000 rows
17 seconds: completed 200000 rows
30 seconds: completed 300000 rows
47 seconds: completed 400000 rows
69 seconds: completed 500000 rows
92 seconds: completed 600000 rows
121 seconds: completed 700000 rows
153 seconds: completed 800000 rows
188 seconds: completed 900000 rows
228 seconds: completed 1000000 rows
271 seconds: completed 1100000 rows
318 seconds: completed 1200000 rows
368 seconds: completed 1300000 rows
422 seconds: completed 1400000 rows
480 seconds: completed 1500000 rows
540 seconds: completed 1600000 rows
605 seconds: completed 1700000 rows
674 seconds: completed 1800000 rows
746 seconds: completed 1900000 rows

Your code

def import_db_table(chunk_size, offset):

It doesn't look like you need to pass offset to this function. All it does is give you the functionality to read from a given row to the bottom. I would omit it, or at least give it a default value of 0. It also looks like you need connection as one of the variables.

    dfs_ct = []
    j = 0
    start = dt.datetime.now()
    df = pd.DataFrame()

    while True:
        sql_ct = "SELECT * FROM my_table limit %d offset %d" % (chunk_size, offset)
        dfs_ct.append(psql.read_sql_query(sql_ct, connection))

        offset += chunk_size

        if len(dfs_ct[-1]) < chunk_size:
            break

As written, the while loop should stop here. You can also get better performance by making a generator instead of a list out of the query results. For example:

Code suggestions

    def generate_df_pieces(connection, chunk_size, offset = 0):
        while True:
            sql_ct = "SELECT * FROM my_table limit %d offset %d" % (chunk_size, offset)
            df_piece = psql.read_sql_query(sql_ct, connection)

            # don't yield an empty data frame
            if not df_piece.shape[0]:
                break
            yield df_piece

            # don't make an unnecessary database query
            if df_piece.shape[0] < chunk_size:
                break

            offset += chunk_size

Then you can call:

    df = pd.concat(generate_df_pieces(connection, chunk_size, offset=offset))

The function pd.concat can take a sequence. Making the sequence be a generator like this is more efficient than growing a list, as
you don't need to keep more than one df_piece in memory until you actually make them into the final, larger one.

Back to your code

        df = pd.concat(dfs_ct)

You're resetting the entire dataframe each time and rebuilding it anew from the whole list! If this were outside of the loop it would make sense.

        # Convert columns to datetime
        columns = ['col1', 'col2', 'col3','col4', 'col5', 'col6',
                   'col7', 'col8', 'col9', 'col10', 'col11', 'col12',
                   'col13', 'col14', 'col15']

        for column in columns:
            df[column] = pd.to_datetime(df[column], errors='coerce')

        # Remove the uninteresting columns
        columns_remove = ['col42', 'col43', 'col67','col52', 'col39', 'col48','col49', 'col50', 'col60', 'col61', 'col62', 'col63', 'col64','col75', 'col80']

        for c in df.columns:
            if c not in columns_remove:
                df = df.drop(c, axis=1)

This part could be done in the loop / generator function or outside. Dropping columns is a good thing to place inside as then the big dataframe you build won't ever need to be larger than you want. If you're able to put only the columns you want in the SQL query, that would be even better, as it would be less to send over the connection.

Another point to make about df.drop is that by default it makes a new dataframe. So use inplace = True so you don't copy your huge dataframe. And it also accepts a list of columns to be dropped:

Code suggestions

        df.drop(columns_remove, inplace = True, axis = 1)

gives the same result without looping and copying df over and over. You can also use:

        columns_remove_numbers = [ ... ] # list the column numbers
        columns_remove = df.columns[columns_remove_numbers]

So you don't have to type all those strings.

Back to your code

        j+=1
        print('{} seconds: completed {} rows'.format((dt.datetime.now() - start).seconds, j*chunk_size))

If you use the generator function version of this, you could put this inside that function to keep track of the performance.


Mapping enum to enum I need to map from one enum type to another.  Here is the enum I am
given:

enum CfgFruitType { CFGNoFruit, CFGApple, CFGApricot, CFGBanana,
   CFGMango, CFGKiwi, CFGFig, CFGGrape, CFGMelon, CFGLemon,
   CFGNectarine, CFGPineapple, CFGWatermelon, CFGCherry, CFGPear,
   CFGStrawberry };

and I need to map this to a second enum:

enum FAddressType { AddressTypeUnknown, AddressTypeCherry,
  AddressTypeApple, AddressTypePear, AddressTypePlum,
  AddressTypeStrawberry };

Here is my implementation:

FAddressType maptype(CfgFruitType cfgtype) {
   int mapper[16][2] =
   {
      { CFGNoFruit,          AddressTypeUnknown },
      { CFGApple,            AddressTypeApple },
      { CFGApricot,          AddressTypeUnknown },
      { CFGBanana,           AddressTypeUnknown },
      { CFGMango,            AddressTypeUnknown },
      { CFGKiwi,             AddressTypeUnknown },
      { CFGFig,              AddressTypeUnknown },
      { CFGGrape,            AddressTypeUnknown },
      { CFGMelon,            AddressTypeUnknown },
      { CFGLemon,            AddressTypeUnknown },
      { CFGNectarine,        AddressTypeUnknown },
      { CFGPineapple,        AddressTypeUnknown },
      { CFGWatermelon,       AddressTypeUnknown },
      { CFGCherry,           AddressTypeCherry },
      { CFGPear,             AddressTypePear },
      { CFGStrawberry,       AddressTypeStrawberry }
     };

   return cfgtype < 16 ? (FAddressType)mapper[cfgtype][1] : AddressTypeUnknown;
}

Is this the best approach?  How can it be improved?

You made a look-up table. Here's 2 things to note:

Your look up table is between 2 types, but your table is only 1 type. You break type safety. They are enums and it's 'safe' if the enums have the same underlying type, but dirty either way.
A switch frequently generates a look up table for you, and in this case definitely     will on any decent compiler. It will not generate an if-if else-else chain. See the     following:

:

FAddressType maptype(CfgFruitType cfgtype)
{
    switch(cfgtype)
    {
    case CFGNoFruit:    return AddressTypeUnknown;
    case CFGApple:      return AddressTypeApple;
    case CFGApricot:    return AddressTypeUnknown;
    case CFGBanana:     return AddressTypeUnknown;
    case CFGMango:      return AddressTypeUnknown;
    case CFGKiwi:       return AddressTypeUnknown;
    case CFGFig:        return AddressTypeUnknown;
    case CFGGrape:      return AddressTypeUnknown;
    case CFGMelon:      return AddressTypeUnknown;
    case CFGLemon:      return AddressTypeUnknown;
    case CFGNectarine:  return AddressTypeUnknown;
    case CFGPineapple:  return AddressTypeUnknown;
    case CFGWatermelon: return AddressTypeUnknown;
    case CFGCherry:     return AddressTypeCherry;
    case CFGPear:       return AddressTypePear;
    case CFGStrawberry: return AddressTypeStrawberry;
    default:            assert(!"Not a valid CfgFruitType!"); return AddressTypeUnknown;
    }
}

Shorter code, very clear, easy to add new cases, very likely to outperform (by a negligible margin) your code.

Or, you could be less explicit and make it really short:

switch(cfgtype)
{
case CFGApple:      return AddressTypeApple;
case CFGCherry:     return AddressTypeCherry;
case CFGPear:       return AddressTypePear;
case CFGStrawberry: return AddressTypeStrawberry;
default:            return AddressTypeUnknown;
}


Python nested json parsing and splitting the values I am trying to split and assign the url's to the variable, I am getting the desired result but I know there is a way where I can improvise the current code.

JSON FILE

{
"Result": [
"Url::Link::Url1",
"Url::Link::Url2",
"Url::Link::Url3",
"Url::Link::Url4",
"Url::Link::Url5",
"Url::Link::Url6",
"Url::Link::Url7"
],
"Record": [
"Record::Label::Music1",
"Record::Label::Music2",
"Record::Label::Music3"
],
}

import requests
import json

url = "http:mywebsite.com"

headers = {
    'User-Agent': "PostmanRuntime/7.15.2",
    'Accept': "*/*",
    'Cache-Control': "no-cache"
    }

result= requests.get("GET", url, headers=headers).json()
url1 = []
url2 = []
url3 = []

for i in result['Result'][0:1]:
    url1.append(i.split('::')[2])

for i in result['Result'][1:2]:
    url1.append(i.split('::')[2])

for i in result['Result'][2:3]:
    url1.append(i.split('::')[2])

Output
url1=Url1
url2=Url2
...

As long as there's nothing special about the nth URL, you can just store the URLs in one list:

urls = []

for result_string in result["Result"]:
    url = result_string.split("::")[2]
    urls.append(url)

If there is something unique information you want to capture about the nth URL, you could store the URLs in a dictionary to map the URL to its type.

def url_type(n):
    """returns the type of url given its index n in the response"""

    #example
    if n == 1:
        return 'url-type-a'

    return 'url-type-b'

urls = {
    "url-type-a": [],
    "url-type-b": []
}

for i, result_string in enumerate(result["Result"]):

    url = result_string.split("::")[2]
    urls[url_type(i)].append(url)


Find missing numbers in a sorted array Array of size \$(n-m)\$ with numbers from \$1..n\$ with \$m\$ of them missing. Find all of the missing numbers. The array is sorted.

  Example: If arr = [1,2,4,5,6,8], result should be 3 & 7.

Here is my code which I would like to get reviewed:

import java.util.*;

class FindMissingNumsArray {

    private List<Integer> missing = new ArrayList();

    private void findMissingNums(int[] array) {
        if(array == null)
            return;
        int length = array.length - 1;
        if(length <= 1)
            return;

        getMissings(array, 0, length);
    }

    private void getMissings(int[] array, int lIndex, int hIndex) {
        if(hIndex - lIndex == 2) {
            fillMissings(array, lIndex, lIndex, hIndex);
            return;
        }
        if(lIndex == hIndex)
            return;

        int i = lIndex;
        int j = hIndex;
        int mid = lIndex + (hIndex - lIndex) / 2;
        getMissings(array, i, mid);
        getMissings(array, mid + 1, j);
        fillMissings(array, i, mid, j);
    }

    private void fillMissings(int[] array, int lIndex, int mid, int hIndex){
        int i = lIndex;
        int j = mid;
        int k = hIndex;
        int diff;
        while(i < mid) {
            diff = array[i + 1] - array[i];
            for(int c = 1; diff > 1; diff--) {
                if(!missing.contains(array[i] + c))
                    missing.add(array[i] + c);
                c++;
            }
            i++;
        }
        while(j < k) {
            diff = array[j + 1] - array[j];
            for(int a = 1; diff > 1; diff--) {
                if(!missing.contains(array[j] + a))
                    missing.add(array[j] + a);
                a++;
            }
            j++;
        }
    }

    private void printMissingList() {
        for(int i: missing)
            System.out.print(i + " ");
    }

    public static void main(String[] args) {
        int[] array = {1, 4, 5, 6, 9, 10, 12, 15, 19};
        int[] array1 = {0, 5, 6, 9, 10};
        FindMissingNumsArray findMissings = new FindMissingNumsArray();
        findMissings.findMissingNums(array);

        System.out.print("Missing numbers in array are: ");
        findMissings.printMissingList();

        findMissings.missing = new ArrayList();
        findMissings.findMissingNums(array1);

        System.out.print("Missing numbers in array1 are: ");
        findMissings.printMissingList();
    }
}

I know there are ways to implement this better in terms of logic, run time, data structures, etc. It will be nice to know what I can do to improve the code.

Sometimes (actually, normally), simpler is better.

I'm going to present a different way of doing this that's really simple, if you understand the tricks, then I'm going to explain what makes it better. One of the reasons I am doing it this way is because your code is hard to follow.... simply because there's so much. I suspect that when you see a different way of doing things a penny will "drop".

Here's the alternative:

private static final int[] findMissing(int[] data) {
    if (data == null || data.length <= 1) {
        // nothing missing.
        return new int[0];
    }
    int first = data[0];
    int last = data[data.length - 1];
    int[] missing = new int[last - first - data.length + 1];
    int missingCursor = 0;
    int expect = first;
    for (int value : data) {
        while (expect < value) {
            missing[missingCursor] = expect;
            missingCursor++;
            expect++;
        }
        expect++;
    }
    return missing;
}

Is that it? Yes, and it is probably still too much. But, let's consider the problem...

for too-small inputs, we know that there's nothing missing.
for larger inputs, we know exactly how much is missing - the difference between the first and last data values, less the number of values we actually have, and, add 1 because the last-minus-first is actually 1 too small.
then, all we have to do is 'expect' certain values, and fill in the gaps until we get the values we expect.

So, that is what the code does, but, note the following:

there is no need for object-oriented programming - there is no object here to represent. A single static method works fine.
there is no need for an adjustable-size ArrayList<Integer>() because we know how much is missing. Also, that's converting int values to Integer members in a List, which is more work than needed.
the method assumes the inputs are valid - no error-handling or validation

Now, that's the way I would write that method, and why I think it is an improvement.

Having said that, I want to point out some other issues in the code:

When creating a collection like an ArrayList, you need to use the diamond operator <> on the right-hand side. The code:

private List<Integer> missing = new ArrayList();

should be:

private List<Integer> missing = new ArrayList<>();

On the other hand, I really like that you have made the left-side List, this shows an understanding of the Collections API
When I run your code, the order of the missing values in the first array is suspect. I get: 2 3 7 8 11 16 17 18 13 14 (yes, the 13 and 14 are at the end for some reason). There's a bug in there somewhere I can't find.
Your code should print a new line after each output. Currently it is all on one line. System.out.println() to do that.

See this running in Ideone


using system calls to compare text files I am trying to make a script that compares 2 .txt files.

It works, but I wanted to double-check if there's something I missed about these system calls (since I'm new at this). Is there any improvement you would've done or anything else you think can help me improve with it (especially with this mmap thing)?

The script returns 2 if it's the same file and 1 otherwise.

#include<sys/stat.h>
#include<unistd.h>
#include<stdio.h>
#include<stdlib.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

int main(int argc , char * argv[])
{
    if(argc != 3)
    {
        printf("something wrong with variables\n");
        exit(-1);
    }

    char* filename1 = argv[1];
    char* filename2 = argv[2];
    char* addr1;
    char* addr2;
    struct stat stat_p1;
    struct stat stat_p2;
    int fileSize1=0,fileSize2=0;

    if(stat(filename1,&stat_p1) == -1)
    {
        printf("error occurred while attempting to stat %s\n" , filename1);
        exit(-2);
    }

    if(stat(filename2,&stat_p2) == -1)
    {
        printf("error occurred while attempting to stat %s\n" , filename2);
        exit(-2);
    }

    if((fileSize1=stat_p1.st_size) != (fileSize2=stat_p2.st_size)) //checks if the size is different than its not the same file.
    {
        return 1;
    }
    else
    {
        int fd1,fd2;
        if((fd1 = open(filename1 ,O_RDONLY)) < 0)
        {
            printf("error opening file %s\n",filename1);
            exit(-3);
        }
        if((fd2 = open(filename2 ,O_RDONLY)) < 0)
        {
            printf("error opening file %s\n",filename2);
            exit(-3);
        }

        addr1 = mmap(NULL, fileSize1+ 1, PROT_READ,MAP_PRIVATE, fd1, 0);
        if (addr1 == MAP_FAILED)
        {
            printf("mmap failed\n");
            exit(-4);
        }

        addr2 = mmap(NULL, fileSize2+ 1, PROT_READ,MAP_PRIVATE, fd2, 0);
        if (addr2 == MAP_FAILED)
        {
            printf("mmap failed\n");
            exit(-4);
        }
        for(int i=0;i<fileSize1+1;i++)
        {
            if(addr1[i] != addr2[i])
            {
                return 1;
            }
        }
        close(fd1);
        close(fd2);
    }
    return 2;
}

(It's in the main file because it's meant to be used as part of testing program later.)

Only for the pedantic

With non-2's complement, the below will compare +0 as matching -0.

char* addr1;
char* addr2;
...
if(addr1[i] != addr2[i]) ... // Oops. +0 == -0

Instead use unsigned char *

unsigned char* addr1;
unsigned char* addr2;
...
if (addr1[i] != addr2[i]) ... // OK.  Only one kind of 0

Better yet, use memcmp().

Return values

"... returns 2 if it's the same file and 1 otherwise." --> Not quite.

Code also returns via exit() with other values.

exit(-1);
exit(-3);
exit(-4);

+ 1??

Off by 1.  No need for + 1.  G. Sliepen.

// for(int i=0;i<fileSize1+1;i++)
for (int i=0; i<fileSize1; i++)

Matching type

.st_size is typed as off_t.  I'd expect fileSize1, fileSize2, i as the same.

off_t fileSize1;
...
for (off_t i = 0; i < fileSize1; i++)

Big files

.st_size is a off_t, some signed type.  mmap(void *addr, size_t length, ...) takes a length of size_t, some unsigned type.

To do this right, code may need to call mmap() multiple times as file size can exceed SIZE_MAX.

Something like:

#define CHUNK  (1048576 /* 1 Meg */)

off_t fileSize1;
fileSize1 = stat_p1.st_size;

....

for (off_t offset = 0; offset < fileSize1; offset += CHUNK) {
  size_t map_size1 = CHUNK;
  if (fileSize1 - offset < CHUNK) {
    map_size1 = (size_t) (fileSize1 - offset);
  }

  addr1 = mmap(NULL, map_size1, PROT_READ,MAP_PRIVATE, fd1, offset);

  // as above for map_size1, addr2

  for (size_t i = 0; i < map_size1; i++)
    ...
  // or
  if (memcmp(addr1, addr2, map_size1) ....


Rewriting file names according to the available screen space I created a function that rewrites file names according to the available space on screen:

@{
  var index = 0;
  foreach (var item in Model)
  {

        var id = "filename_" + ++index;
        @Html.SpanFor(i => item.FileName, new { id = id, title = item.FileName, style = "white-space: nowrap;" })

  }
}

@section Scripts
{

}

Here are the results before and after running script:

Before:

After:

The function runs quite fast for now, but should I be worried from future impact? Any other suggestion or alternatives? Optimization?

I thought about splitting it to 3 spans, 1 for filename, second for ..., and 3rd for the extension, but I don't really know how to make the overflow of the first be hidden under the other two. Besides, if a truncation isn't required, it's gonna look weird.

In general case I would to get text_width and container_width, then calculate new_string_length=container_width/text_width*length, than setup increase or decrease text, depending on result_text_width > container_width  and do the loop like yours.

This is really needed only if strings have a very big size.


New Game - Am I going down the right path for DOM manipulation? I find myself using a lot of .find functions within what I'm developing. The reason being that I need to run this same function on a TON of different characters eventually, and the easiest way I could find to do this was make appropriate sections classes and wrap each character in their respective ID.

Is using .find a ton (to only target the classes within the #characterId) considered bad practice?

JS Fiddle Example: http://jsfiddle.net/hgwNU/6/

Example Script:

(x is the internal javascript object, where y is the DOM ID for what to target)

function updateDom(x,y){
    if(x.pc < 1 || x.speed >= 4){
        y.find('.addSpeed').attr('disabled', 'disabled');
    }
    if(x.speed <= 1){
        y.find('.remSpeed').attr('disabled', 'disabled');
    }
    if(x.speed > 1){
        y.find('.remSpeed').removeAttr('disabled');
    }
    if(x.speed < 4 && x.pc > 0){
        y.find('.addSpeed').removeAttr('disabled');
    }
    y.find(":checkbox").each(function() {
        if($(this).data('pc') > x.pc && $(this).prop('checked')==false){
            $(this).attr('disabled', 'disabled');
        } else if ($(this).data('pc') <= x.pc) {
            $(this).removeAttr('disabled');
        }
    });
    for (var key in x) {
        y.find('#' + key).text(x[key]);
    };
}

Called with:

updatePlayer(bulk,$('#bulkUi'));

JS Fiddle Example: http://jsfiddle.net/hgwNU/6/

I suppose I'm just curious if I'm off to a good start for how to manipulate both the GUI and the internal object effectively, easily, quickly and repeatedly. Looking for best practices if anybody has any feedback thus far.

Personally instead of using .find I prefer passing context to the jQuery selector like this;

$('.remSpeed', y);

Which will find all instances of .remSpeed that are within the element y.


Interface design to have multiple implementations of network call libraries (Retrofit or Volley) I am learning design patterns from Head First Design Patterns and a principle I came across is to "code to interface". Now I want to apply this principle in my Android app, which includes lots of API calls. I am using Retrofit for network calls.

I have applied this principle considering some scenario something like this:

Suppose in the future I need to shift from Retrofit to Volley or any other networking library.
I created an interface with a method to getData from the server and created a class which implements that interface. The implemented method includes the Retrofit code to getData from the server.
Now in the future, if I want to use Volley, I'll create a new class implementing the interface and with Volley code to retrieve data from the server.

Interface

public interface NetworkCallsApi {
    String getDataFromServer(Activity activity,String url,String Callback);
}

Retrofit Implementation

public class NetworkCallsRetrofitImpl implements NetworkCallsApi{

private Retrofit retrofit;

@Override
public String getDataFromServer(Activity activity, String url, String Callback) {

retrofit = new Retrofit.Builder()
        .baseUrl(StringConstants.ROOT_URL)
        .addConverterFactory(GsonConverterFactory.create())
        .build();

RetrofitApi apiCalls = retrofit.create(RetrofitApi.class);

Call<ResponseBody> call = apiCalls.getDataFromServer(url);

call.enqueue(new Callback<ResponseBody>() {
    @Override
    public void onResponse(Call<ResponseBody> call, Response<ResponseBody> response) {

    }

    @Override
    public void onFailure(Call<ResponseBody> call, Throwable t) {

    }
});
return null;
}
}

ServiceCalls.java

public class ServiceCalls {

    public static ServiceCalls serviceCalls;
    public NetworkCallsApi networkCallsimpl;

    public static ServiceCalls getInstance(){

        if(serviceCalls == null){
            serviceCalls = new ServiceCalls();
        }
        return serviceCalls;
    }

    public void setNetworkCallsimpl(NetworkCallsApi networkCallsimpl) {
        this.networkCallsimpl = networkCallsimpl;
    }

    public String getDataFromServer(Activity activity, String url, String callback){
        networkCallsimpl.getDataFromServer(activity,url,callback);
        return null;
     }
 }

MainFragment:

public class StatusFragment extends Fragment {

    @Override
    public void onCreate(@Nullable Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        //Setting the implementation
        ServiceCalls.getInstance().setNetworkCallsimpl(new NetworkCallsRetrofitImpl());
    }

    @Nullable
    @Override
    public View onCreateView(LayoutInflater inflater, @Nullable ViewGroup container, @Nullable Bundle savedInstanceState) {

        view = inflater.inflate(R.layout.fragment_pnr_status,container,false);

        try{
          init();
        }catch(Exception e){}
        return view;
   }

    private void init() {

        ok.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View view) {

            setUrlKeysAndValues();
            ServiceCalls.getInstance().getDataFromServer(getActivity()
                ,Utils.getInstance().buildUrl(urlKeys,urlValues),"onResponse");
        }
        });

    }
}

Have I correctly applied that principle? If not, what changes do I need to make? In onCreate of MainFragment, I am hardcoding the implementation. How can the interface design pattern be better applied in this scenario?

Let me point out an other aspect, you might miss at the moment. I know that this is a learner question/example, but keep in mind that your approach is designing an application where you can switch the network library (in extreme) at runtime. This is not the most common scenario.

Actually it is more likely that you want to switch Retrofit against a mock or Volley against a mock in your tests. So you have done everything right, if you can write tests for related parts of your system (e.g. the StatusFragment)  without doing real network calls and there is no test switch in your production code. (The network stuff will get it's own test.)

On the other hand, although I'm not familiar with the libraries, Google told me that both seems to have some own mocking concepts. So there might not even be a need to switch your implementation in unit tests. In case you finally end up with only one implementation now and optional a total replacement in the future, this approach might add an unnecessary (mental and technical) overhead.

Implementing against an interface don't necessary mean a Java interface. If you only have one implementation at a time, it is totally fine to drop NetworkCallsApi and move NetworkCallsRetrofitImpl into ServiceCalls. Afterwards you are implementing against the interface of ServiceCalls and you don't care about is implementation. It is also pretty straight forward to change the implementation in ServiceCalls later without changing other parts of the application.


Static variable representing program state accessed with multiple functions I have this method that performs certain functions based on the state that the algorithm is currently at:

static infoStruct_t info;

void first_part_of_algorithm(void){
    info.state = FIRST_STATE;
    //set other information here as well
    while(info.state < FINAL_STATE){
        switch(info.state){

            case FIRST_STATE:
                perform_first_state_function();
                break;

            case SECOND_STATE:
                perform_second_state_function();
                break;

            case THIRD_STATE:
                perform_check_needed_for_algorithm();
                break;

            default:
                //should never be reached
                break;

        }
    }

}

Now in each of these "perform" functions, the state can change. The first function always causes the state to go to the second state, the second function always makes the state go to the third state, and the third function will either reset the state to the first state, or go to the final state and leave the first part of the algorithm. Also, different parts of the info struct are set at various points in any of the functions.

I have not yet written the second part of the algorithm, but it will need to have access to the info struct

Here are the following state / check functions:

void perform_first_state_function(void){
    while(wait_for_condition()!=SUCCESS){
        sleep();
    }
    info.state = SECOND_STATE;
    sleep();
}

void perform_second_state_function(void){
    int attempts = 0;
    while(attempts < ALLOWED_ATTEMPTS){
        if(event_is_ready()){
            if(info.detection_number==0){
                //set various info properties
                ...
                info.first_detection_time = get_time();
                info.detection_number = 1;
                break;
            }
            else{
                //set other info properties
                ...
                info.second_detection_time = get_time();
                info.state = THIRD_STATE;
                break;
            }
        }
        attempts++;
        sleep();
    }
}

My questions are:

Is this a code smell that the functions are implicitly accessing the static info variable?
Should I explicitly pass in the info struct as a pointer? (for readability)
Does the info struct need to be a singleton?
Any general suggestions?

Since it's a single-threaded algorithm, I think you could simplify things a bit to avoid all the state-checking. Unless you're reading the state from another thread, you don't really need to keep track of it at all. You could simply make perform_check_needed_for_algorithm return a bool:

for (;;)
{
    /* info.state = FIRST_STATE; */
    perform_first_state_function();
    /* info.state = SECOND_STATE; */
    perform_second_state_function();
    /* info.state = THIRD_STATE; */
    if (perform_check_needed_for_algorithm())
    {
        /* info.state = FINAL_STATE; */
        break;
    }
}

The commented-out state assignments are just there to show what's happening at each stage. Although a state machine's inner functions ought to be responsible for updating the state, in an example as simple as this, it's arguably more readable to update the state in the outer function.

If you need access to the state from another thread, then you'll need to update it using atomic operations, and mark it as volatile.

Afterthought:
perform_second_state_function doesn't always advance the state. It fails once ALLOWED_ATTEMPTS is exceeded. However, it dumps you back into the outer loop, and dives straight back into perform_second_state_function because state.info is still SECOND_STATE. Bit of a design error there IMO - it will keep trying no matter what.