Is this a correct Factory Method implementation? I'm learning about design pattern and I tried to implement a Factory Method example, based on the GoF book.

Can I say that this is a correct implementation of it ? If not I will be glad to find out what are the pitfalls.

#include <iostream>
#include <vector>
using namespace std;

class Widget {
public:
    virtual void draw() = 0;
};

class Win7Widget : public Widget {
public:
    void draw() { cout << "Win 7 widget" << endl; }
};

class Win8Widget : public Widget {
public:
    void draw() { cout << "Win 8 widget" << endl; }
};

class Factory {
public:
    virtual Widget* Create() = 0;
    virtual ~Factory() {}
};

class Win7Factory: public Factory {
public:
    Widget* Create() { return new Win7Widget; }
};

class Win8Factory: public Factory {
public:
    Widget* Create() { return new Win8Widget; }
};

int main()
{
    unique_ptr<Factory> win7fact(new Win7Factory);
    unique_ptr<Factory> win8fact(new Win8Factory);

    vector<Widget*> widgets;
    widgets.push_back(win7fact->Create());
    widgets.push_back(win8fact->Create());

    for(const auto& f : widgets)
    {
        f->draw();
    }

    return 0;
}

Don't use dynamic allocation if automatic objects will do:

unique_ptr<Factory> win7fact(new Win7Factory);
unique_ptr<Factory> win8fact(new Win8Factory);

Should be:

Win7Factory win7fact;
Win8Factory win8fact;


Clickable icon for expanding/collapsing content The general idea of the code is that I have a + or - icon (".trigger") that is click-able and will expand or collapse the content (".cont") directly following it (there are many of there expand/collapse pairs).  I also have a span("#expandAll") that, when clicked, will expand or collapse every block.

Please review my first usage of jQuery and help me put my head into a more jQuery state.  Where can the code be improved?  Are there any problems with it?  Is there a better way to do it?

(To be perfectly honest, I have no idea what the $() function really does.)

var expandAll = function () {
    $(".cont").removeClass("hid");
    $("#expandAll").html("Colapse All");
    $("#expandAll").unbind("click", expandAll);
    $(".trigger").html("-");
    $("#expandAll").click(colapseAll);
}
var colapseAll = function () {
    $(".cont").addClass("hid");
    $("#expandAll").html("Expand All");
    $("#expandAll").unbind("click", colapseAll);
    $(".trigger").html("+");
    $("#expandAll").click(expandAll);
}
$(document).ready(function () {
    $(".trigger").click(function (event) {
        $this = $(this);
        $this.html($this.text() == '+' ? '-' : '+');
        $($this.nextAll(".cont").get(0)).toggleClass("hid");
    });
    $("#expandAll").click(expandAll);
});

Please let me know if you need more/less info.

I've refactored your code and added comments to explain certain things.

// We can shorten this from document.ready(...) to $(...)
// Internally, jQuery will add the passed in function to a list
// of handlers that will be invoked when the document is ready.
$(function() {
    // Since the $ function will query the document with the
    // specified selector we want to cache these.
    // Doing this from within the ready callback allows us
    // to keep these references hidden.
    // I've also added the 2 function variables to keep them
    // hidden as well.
    var expandAll, colapseAll,
        expandAllElement = $("#expandAll"),
        contElements = $(".cont"),
        triggerElements = $(".trigger");

    expandAll = function() {
        contElements.removeClass("hid");
        triggerElements.html("-");
        // Notice how we can chain these calls together?
        // jQuery was designed with a fluent interface.
        expandAllElement.html("Colapse All").one("click", colapseAll);
    };

    colapseAll = function() {
        contElements.addClass("hid");
        triggerElements.html("+");
        expandAllElement.html("Expand All").one("click", expandAll);
    };

    triggerElements.click(function(event) {
        // Always define a variable so the 'name'
        // is not defined in the global object,
        var $this = $(this);
        $this.html($this.text() == '+' ? '-' : '+');
        $($this.nextAll(".cont").get(0)).toggleClass("hid");
    });

    // Using the one function allows you to skip
    // unbinding the event handler for each click.
    expandAllElement.one("click", expandAll);
});


Getting a monitor packet The following are an example of methods supported by a proprietary device:

Monitor
ControlTemp
PutPeakInfo
GetPeakInfo

I have a class that builds the packets for the above corresponding methods:

GetMonitorPacket
GetControlTempPacket
GetPutPeakInfo

Example:

public byte[] GetMonitorPacket(int ddcNum)
    {
        byte[] monitorPacket = new byte[MONITOR_PACKET_BYTE_COUNT];
        Buffer.BlockCopy(START_HEADER_BYTES, 0, monitorPacket, 0, START_HEADER_BYTES.Count());
        monitorPacket[2] = (byte) ddcNum;
        monitorPacket[3] = 0x01;
        byte checksum = 0;
        foreach (var b in monitorPacket)
        {
            checksum = (byte) (((ushort) checksum + (ushort) b) & 0xff);
        }
        checksum = (byte) (checksum ^ 0x55);
        monitorPacket[4] = checksum;
        return monitorPacket;
    }

All they do is return the bytes in proper format, which then will be sent to the proprietary device via TCP to call the methods. Now I'm screwed on my naming convention for GetPeakInfo method.

How do I name this? What about GetGetPeakInfoPacket (two gets for a method name)?
Do I break the naming convention here as an exception case and drop one of the gets?
Do I use some other naming convention?

Actually, GetPutPeakInfo sounds a little bit weird too, but at least there's no duplication.

How about using Generate as prefix?

GenerateMonitorPacket
GenerateControlTempPacket
GeneratePutPeakInfoPacket
GenerateGetPeakInfoPacket

Alternatively, you can also create a separate class for each type of packet generator. You've only shown the code to create the packet for Monitor, so I don't know how complex the code for the other packets is, but separating these four methods into their own class might be logical and beneficial in the long run. If you create separate classes, you put the name of the packet (Monitor, ControlTemp, PutPeakInfo and GetPeakInfo) in the class name.

public class MonitorPacketGenerator
{
    public byte[] Generate(int ddcNum) { ... }
}


Summing over the values of a dict I'm coming from the Java world and making my first steps on Python and see how good it is to express more with less code. I'd like to know if, via Python, the size of the following code could be reduced:

column_entropy = 0
for k in frequencies.keys():
    p = frequencies[k] / 10
    column_entropy += p * log(p, 2) if p > 0 else 0
column_entropy = -column_entropy

The formula for calculating entropy is:

$$ \sum_i p_i \log\frac{1}{p_i} $$

or \$-\sum p_i \log p_i\$. It would be nice if the code could express that.

probabilities = [f / 10 for f in frequencies.values() if f > 0]
column_entropy = -sum(p * log(p, 2) for p in probabilities)

The second line is the goal — it's a direct translation of the formula.  The first line is the setup that makes it possible to write the second line nicely.


Basic C++ object class I have implemented a basic object class in C++, and I want to make sure that I have implemented everything efficiently and safely, as I made a few mistakes while making it, which I corrected, but I could have left a few more mistakes behind:

class object
{
private:
    class dummy
    {
    public:
        dummy()
        {
        }
        virtual ~dummy()
        {
        }
        virtual const std::type_info &type() = 0;
        virtual dummy *duplicate() const = 0;
        virtual bool eq(object &) = 0;
    };

    template < typename _Ty > class data : public dummy
    {
    public:
        data()
        {
        }

        data(const _Ty &_Value)
            : __data(_Value)
        {
        }

        ~data()
        {
        }

        const std::type_info &type()
        {
            return typeid(_Ty);
        }

        data *duplicate() const
        {
            return new data<_Ty>(__data);
        }

        bool eq(object &_Obj)
        {
            return _Obj.cast<_Ty>() == __data;
        }

        _Ty __data;
    };

    dummy *d;
public:
    object()
    {
    }

    template < typename _Ty > object(const _Ty &_Value)
        : d(new data<_Ty>(_Value))
    {
    }

    object(const object &_Obj)
        : d(_Obj.d->duplicate())
    {
    }

    ~object()
    {
        if (!empty())
        {
            delete d;
        }
    }

    const std::type_info &type()
    {
        return (empty() ? typeid(void) : d->type());
    }

    object &operator=(object &_Rhs)
    {
        d = _Rhs.d->duplicate();
        return *this;
    }

    object &swap(object &_Rhs)
    {
        std::swap(*this, _Rhs);
        return *this;
    }

    template < typename _Ty > object &operator=(const _Ty &_Value)
    {
        d = new data<_Ty>(_Value);
        return *this;
    }

    template < typename _Ty > _Ty cast()
    {
        if (type() == typeid(_Ty))
        {
            return static_cast<data<_Ty> *>(d)->__data;
        }
        throw std::exception("Invalid cast type");
    }

    bool operator==(object &_Rhs)
    {
        return (type() == _Rhs.d->type() ? d->eq(_Rhs) : false);
    }

    template < typename _Ty > bool operator==(_Ty _Value)
    {
        return (type() == typeid(_Ty) ? cast<_Ty>() == _Value : false);
    }

    bool operator!=(object &_Rhs)
    {
        return !(*this == _Rhs);
    }

    template < typename _Ty > bool operator!=(const _Ty &_Value)
    {
        return !(*this == _Value);
    }

    bool empty()
    {
        return !d;
    }
};

As per common naming convention, the class names should start with a capital letter.
It may be more readable to put the template line above the class line:

template < typename _Ty >
class data : public dummy
{
}

Your bool functions should be const.  Any member function that doesn't modify data members should be const.  Such functions also include accessors ("getters") and the bool operators (operator==, operator!=, operator<, and operator>).

For instance, with empty():

bool empty() const
{
    // ...
}

and operator==:

bool operator==(object &_Rhs) const
{
    // ...
}

Also, the above parameter should be const& since _Rhs is not being modified.
If you're not defining your own constructor and/or destructor, you don't need to make them yourself.  The compiler will make them for you.

However, if you're using C++11, you can have an explicitly defaulted constructor and destructor:

data() = default;
~data() = default;


Excel VBA to create a visual timeline of events using an XY scatter chart I have details of key decisions made by a business in a worksheet "Decision Record."

My data is laid out as follows:

I have written some code to create a timeline of these decisions using an XY scatter chart, like this one:

The code works fine, but as with most of the code I write, it doesn't look tidy to me. Perhaps it's because of the long with statement, which seems like it could be simplified, maybe with a separate with statement for the DataLabels method.

Is this good practice? Is there something else I should be doing to tidy this up?

Sub UpdateDecisionTimelineChart()

    Dim scount As Integer
    Dim labelrotation As Integer
    Dim c As Range

    ActiveSheet.ChartObjects("chtDecisionTimeline").Activate
    scount = 0
        For Each c In Range(Worksheets("Decision Record").Range("C7"), Worksheets("Decision Record").Range("C7").End(xlDown))
        scount = scount + 1
        ActiveChart.SeriesCollection.NewSeries
        With ActiveChart.SeriesCollection(scount)
            .Name = "='Decision Record'!" & c.Offset(0, 1).Address
            .XValues = "='Decision Record'!" & c.Address
            .Values = "='Decision Record'!" & c.Offset(0, -1).Address
            .MarkerStyle = 8
            .MarkerSize = 7
            .MarkerBackgroundColor = RGB(228, 10, 56)
            .MarkerForegroundColor = -2
            .Format.Line.Visible = False
            .ApplyDataLabels
            .DataLabels.ShowValue = False
            .DataLabels.ShowSeriesName = True
            .DataLabels.Position = xlLabelPositionAbove
            .DataLabels.Orientation = -45
        End With
        Next
End Sub

Your variable names could use some tidying up-

Instead of c why not record as they are in the "Decision Record"

labelrotation should be labelRotation - Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

Same goes for scount - sCount or even recordCount

Integers - integers are obsolete. According to msdn VBA silently converts all integers to long.

Be sure to avoid things like .Activate - it just slows the code down by needing to fiddle with the spreadsheet while doing everything else behind the scenes. There's a good question on StackOverflow addressing this - https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros .

Dim timelineChart As Chart
    Set timelineChart = ActiveSheet.ChartObjects("chtDecisionTimeline")
    ...

        timelineChart.SeriesCollection.NewSeries
        With timelineChart.SeriesCollection(scount)

Speaking of structure, your spacing is a little off - your with block is the same level as your for block. Try pushing the loop back so alignment is correct -

Option Explicit

Sub UpdateDecisionTimelineChart()

    Dim sCount As Long
    Dim labelrotation As Long
    Dim record As Range

    Dim timelineChart As Chart
    Set timelineChart = ActiveSheet.ChartObjects("chtDecisionTimeline")
    sCount = 0
    For Each record In Range(Worksheets("Decision Record").Range("C7"), Worksheets("Decision Record").Range("C7").End(xlDown))
        sCount = sCount + 1
        timelineChart.SeriesCollection.NewSeries
        With timelineChart.SeriesCollection(sCount)
            .Name = "='Decision Record'!" & record.Offset(0, 1).Address
            .XValues = "='Decision Record'!" & record.Address
            .Values = "='Decision Record'!" & record.Offset(0, -1).Address
            .MarkerStyle = 8
            .MarkerSize = 7
            .MarkerBackgroundColor = RGB(228, 10, 56)
            .MarkerForegroundColor = -2
            .Format.Line.Visible = False
            .ApplyDataLabels
            .DataLabels.ShowValue = False
            .DataLabels.ShowSeriesName = True
            .DataLabels.Position = xlLabelPositionAbove
            .DataLabels.Orientation = -45
        End With
    Next
End Sub

The object model for the series object doesn't really specify any of the attributes' defaults, so I think your With block is pretty clean. You might want to create some variables or something like -

Dim decisionRecord As String
decisionRecord = "='Decision Record'!"

        .Name = decisionRecord & record.Offset(0, 1).Address
        .XValues = decisionRecord & record.Address
        .Values = decisionRecord & record.Offset(0, -1).Address

Your xlDown could be fixed like this -

Dim decisionRecordSheet As Worksheet
Set decisionRecordSheet = Worksheets("Decision Record")
Dim lastRow As Long
lastRow = decisionRecordSheet.Cells(Rows.Count, 3).End(xlUp).Row
Dim recordRange As Range
Set recordRange = decisionRecordSheet.Range(Cells(7, 3), Cells(lastRow, 3))
...
For Each record In recordRange

With all these variables using the sheet, why not use the CodeName - Worksheets have a CodeName property - View Properties window (F4) and the (Name) field can be used as the worksheet name. This way you can avoid Sheets("mySheet") and instead just use mySheet.

Unless you need it to be Public you should make it private

Private Sub UpdateDecisionTimelineChart()

Also, I don't see you ever using the labelRotation variable, so unless that part was removed, you don't need it.


object oriented Node.js class RiddleInfo.js So I am trying to make a Object oriented Node.js class for store information about riddles.
Please feel free to be harsh as possible. Would love to hear your ideas about how to make this class better!

function RiddleInfo(riddle, answer, hint1, hint2, hint3, ohFactor, difficulty) {
    this.riddle = riddle || null;
    this.answer = answer || null;
    this.hint1 = hint1 || null;
    this.hint2 = hint2 || null;
    this.hint3 = hint3 || null;
    this.ohFactor = ohFactor || null;
    this.difficulty = difficulty || null;
}

RiddleInfo.prototype.getRiddle = function() {
    return this.riddle;
}

RiddleInfo.prototype.setRiddle = function(riddle) {
    this.riddle = riddle;
}

RiddleInfo.prototype.getAnswer = function() {
    return this.answer;
}

RiddleInfo.prototype.setAnswer = function(answer) {
    this.answer = answer;
}

RiddleInfo.prototype.getHint = function(hintNumber) {
    switch(hintNumber)
    {
        case 1:
            return this.hint1;
            break;
        case 2:
            return this.hint2;
            break;
        case 3:
            return this.hint3;
            break;
        default:
            return 'InvalidArgumentException line 25 in RiddleInfo.js: ' +
                   'hintNumber must be 1, 2, or 3 but instead was ' + hintNumber;
    }
}

RiddleInfo.prototype.getOhFactor = function() {
    return this.ohFactor;
}

RiddleInfo.prototype.setOhFactor = function(ohFactor) {
    this.ohFactor = ohFactor;
}

RiddleInfo.prototype.getDifficulty = function() {
    return this.difficulty;
}

RiddleInfo.prototype.setDifficulty = function(difficulty) {
    this.difficulty = difficulty;
}

RiddleInfo.prototype.isSimilarTo = function(otherRiddle) {
    if (this.getRiddle() === otherRiddle.getRiddle() ||
        this.getAnswer() === otherRiddle.getAnswer()
        ) {
        return true;
    } else {
        return false;
    }
}

RiddleInfo.prototype.fill = function(newFields) {
    for (var field in newFields) {
        if (this.hasOwnProperty(field) && newFields.hasOwnProperty(field)) {
            if (this[field] !== 'undefined') {
                this[field] = newFields[field];
            }
        }
    }
};

module.exports = RiddleInfo;

From the outset, I'm not sure how useful/necessary this constructor ("class") is, to be honest. Everything could seemingly be stored in a simple object literal:

var aRiddle = {
  riddle: "What gets wetter and wetter the more it dries?",
  answer: "A towel",
  difficulty: "meh",
  ohFactor: "so-so",
  hints: [
    "Galactic hitchhikers should always bring one",
    "Look in the bathroom",
    "Everyone has several of the things"
  ]
};

Point is, your class doesn't do any input checking, and most of its methods are getters and setters - which don't necessarily make much sense in JavaScript. For instance. I can do this:

var aRiddle = new RiddleInfo();             // no arguments at all: no errors
aRiddle.riddle = "This statement is false"; // bypasses setter completely
aRiddle.foo = 23;                           // arbitrary property assignment
aRiddle.setAnswer(new Date());              // using setter, but passing in nonsense

The only methods left are getHint and isSimilarTo. Both of these could be handled by functions outside the object. Yes, it defies encapsulation, but... 'eh, I'd choose that over a lot of "pointless" code.

But, let's give the code a once-over. Again, I'd suggest a very different approach, but that's not the same as reviewing the code you posted.

First off, your constructor takes an awful lot of arguments. This creates very tight coupling to any other code: All that other code must know the correct order of all those arguments to use the constructor, which makes it harder to change this constructor in the future without breaking everything.
Now, I'm not sure what arguments you consider required and which ones might be optional, but at the very least, I'd say a riddle consists of a question, and an answer. I'd consider the other arguments optional for now. And to avoid having to order the arguments correctly, I'd use an object literal (like the one above) for all the optional arguments:

function RiddleInfo(question, answer, options) {
  // ... see below ...
}

As mentioned, I'd skip getters and setters unless they actually contributing in some way. For instance, they could (should) check their input, to prevent nonsense values being set. And if you're going to have them, you might as well use them yourself.

function RiddleInfo(question, answer, options) {
  this.setQuestion(question);
  // ...
}

RiddleInfo.prototype.setQuestion = function (question) {
  if( typeof question !== "string" ) {
    throw new TypeError("Question must be a string");
  }
  this.question = question;
};

Now the constructor will use the setter, meaning it's guaranteed to throw a TypeError if you're using it wrong. You can do something similar for the other arguments, as well as the optional ones.

Speaking of: Don't return a string that looks like an exception like you do in getHint. It's basically lying: It's not an "InvalidArgumentException" - it's just a string. And hardcoding the line number in there is crazy - you'll have to change it constantly, even if you've only been adding comments or just whitespace. Just throw a real exception. It'll contain a stack trace, and give you the line number.

Anyway, the above neglects one thing: You can still completely bypass the setter by just writing riddle.question = .... Now, as mentioned, getters and setters are probably more trouble than their worth in JavaScript, but if you want a more "secure" way of doing things, you'll have to change things up a bit:

function RiddleInfo(question, answer, options) {
  // define the setter here
  this.setQuestion = function (newQuestion) {
    if( typeof question !== "string" ) {
      throw new TypeError("Question must be a string");
    }
    question = newQuestion;
  };

  // define the getter here too
  this.getQuestion = function () {
    return question;
  };

  this.setQuestion(question);

  // ... similar thing for other arguments ...
}

The point in the above is that question remains a local variable inside the constructor function. Instead of setting it on the instance (this.question = question), we set the getter/setter functions instead. Thanks to closures, they can access the local question variable. What you get is something like

var aRiddle = RiddleInfo("...", "Darkness");

aRiddle.setQuestion("The more there is, the less you see. What could I be?");
aRiddle.getQuestion(); // => "The more there is [...]"
aRiddle.question = "this won't have any effect";
aRiddle.getQuestion(); // => "The more there is [...]"

Lastly, if you've got more than 1 of something, you should probably use an array: i.e. use an array for the hints. You may want more than 3 or fewer, so don't have named variables for exactly 3.

Phew, I think I'll stop here. Hopefully you've got something to chew on.


Editing system files in Linux (as root) with GUI and CLI text editors My intention is to POSIX-ly write one generalized function for running various text editors I use for different purposes through sudoedit, i.e. editing files as root safely. Safely = for instance, if a power loss occurs during the file edit; another example could be lost SSH connection, etc.

Originally, I had these Bash functions defined for this purpose in my .bash_aliases file:

function sucode
{
    export SUDO_EDITOR='/usr/bin/code --wait'
    sudoedit "$@"
}
function susubl
{
    export SUDO_EDITOR='/opt/sublime_text/sublime_text --wait'
    sudoedit "$@"
}
function suxed
{
    export SUDO_EDITOR='/usr/bin/xed --wait'
    sudoedit "$@"
}

Since yesterday, I'm trying to generalize that solution for other Linux users to be able to take advantage of it. Momentary peek:

# Text editing as root; The proper way through `sudoedit`.
sudoedit_internal()
{
    [ "${#}" -lt 3 ] && { printf '%s\n' 'sudoedit_internal(): Invalid number of arguments.' 1>&2; return; }
    editor_path=$( command -v "${1}" )
    [ -x "${editor_path}" ] || { printf '%s\n' "sudoedit_internal(): The editor path ${editor_path} does not exist on this system." 1>&2; return; }
    editor_wait_option=${2}
    shift 2
    env SUDO_EDITOR="${editor_path} ${editor_wait_option}" sudoedit "${@}"
}
# CLI
suvi()   { sudoedit_internal vi   '' "${@}"; }
sunano() { sudoedit_internal nano '' "${@}"; }
# GUI
sucode() { sudoedit_internal code -w "${@}"; }
susubl() { sudoedit_internal subl -w "${@}"; }
suxed()  { sudoedit_internal xed  -w "${@}"; }

These 5 editors I use. Please take that as an example only.

As I should not update this question any further, you can find the up-to-date version of this script snippet in my Unix & Linux Answer.

It's good form to return non-zero on error.  The not-optional option is a little ugly and an environment variable may work better.

Some extraneous syntax can go:

1 before >&2 is implied
{} around unsubstituted dereferences doesn't add anything
echo is an alias for printf "%s\n"
testing for error instead of success allows test && echo && return without braces
command -v tests validity for you; no need to test again
you've moved the complexity into a function already; reward yourself by using aliases to invoke it

sudoedit_internal()
{
    [ $# -lt 2 ] && echo "sudoedit_internal(): Invalid number of arguments." >&2 && return 1
    ! command -v "$1" >/dev/null && echo "sudoedit_internal(): The editor $1 does not exist on this system." >&2 && return 1
    editor="$1"; shift
    SUDO_EDITOR="$editor $opt" sudoedit "$@"
}

for ed in vi nano       ; do alias su$ed="opt=   sudoedit_internal $ed"; done
for ed in code subl xed ; do alias su$ed="opt=-w sudoedit_internal $ed"; done


Read a Person object from file stream A simple concept yet I seem to have written it in a complicated way.

I have written some C++ source code that should:

read in a file
parse it
save the data to a class
print out all the saved class data

I am absolutely new to C++ and OOP and Programming and English.

#include <iostream>
#include <string>
#include <fstream>
#include <sstream>
#include <vector>

class Person {
private:
    int age;
    std::string name;

public:
    void setAge(int age) {
        this->age = age;
    }

    int getAge() {
        return age;
    }

    void setName(std::string name) {
        this->name = name;
    }

    std::string getName() {
        return name;
    }

    void printInfo() {
        std::cout
            << "Name: " << getName() << '\n'
            << "Age: " << getAge() << '\n';
    }
};

std::istream& operator>> (std::istream& is, Person& person) {
    int age;
    is >> age;

    std::string name;
    is >> name;

    person.setAge(age);
    person.setName(name);

    return is;
}

int main() {
    auto myfilename = "unknown-people.txt";

    std::ofstream myfile(myfilename, std::ios::binary | std::ios::trunc);
    myfile << "3 Baby\n";
    myfile << "48 Linus Torvalds\n";
    myfile << "62 Bill Gates\n";
    myfile << "115 Kane Tanaka\n";
    myfile.close();

    std::ifstream mysamefile(myfilename, std::ios::binary);

    std::string details;
    std::vector<Person> people;

    while (std::getline(mysamefile, details)) {
        Person person;
        std::istringstream personDetails(details);
        personDetails >> person;
        people.push_back(person);
    }

    mysamefile.close();

    for (std::vector<Person>::iterator it = people.begin(); it != people.end(); ++it) {
        it->printInfo();

        if (!(it != people.end() && it == --people.end())) {
            std::cout << "\n\n";
        }
    }
}

You seem to have a good grasp of the concepts - this is looking pretty good for a first effort!
Those accessors (setAge() + getAge(), and setName() + getName()) are exactly equivalent to making the members age() and name() public, which would be simpler and clearer.  I'll assume that you're later going to add some code to check that the values are reasonable (don't allow negative ages, for example, nor empty or whitespace-only names).
Note also that the "get" accessors can be marked const, because they don't change the Person:
int getAge() const {
    return age;
}

std::string getName() const {
    return name;
}

In passing, I'll note that storing age as a member might not be the best design, especially when storing to a file that might be read back several years later.  If we store the year of birth, that's something that won't normally change, and we can always calculate the age from it.

The operator>>() is mostly what I'd expect.  You can chain the reading like this:
int age;
std::string name;
is >> age >> name;

However, there's a bug here.  is >> name will read up to the next whitespace, but our names can have whitespace in them.  We actually want to read up to the next newline, like this:
std::string name;
std::getline(is, name);

Another consideration here is that we don't want to update the person if we failed to read both values (otherwise, operator<<() could be a friend function, and read directly to person.age and person.name).  We can easily test whether the reads succeeded:
if (is) {
    person.setAge(age);
    person.setName(name);
}

When we have a reader, it's a good idea to have a matching writer that produces the same format.  That's very simple to write:
std::ostream& operator<<(std::ostream& os, const Person& person) {
    return os << person.getAge() << ' '
              << person.getName() << '\n';
}

Note that we have a reference to a const person here - the writer is promising not to modify person.

When reading the people in, we don't need to copy each line of text into a buffer (once we've fixed the bug I mentioned with reading names) - we can stream directly from the file stream into the person object:
std::vector<Person> people;

Person person;
while (mysamefile >> person) {
    people.push_back(std::move(person));
}

(The std::move() is a non-essential optimisation - it says we won't be using person until it's re-written using >>).

When displaying the people, we can make the code simpler using range-based for like this:
for (const Person& person: people) {
    person.printInfo();
    std::cout << "\n\n";
}

Of course, this is slightly different, because we're now printing the newlines after the last person too.  A good way to solve this is to print the newlines before each person, but not if it's the first person.  To do this, we can initially set our separator to be an empty string, and update after it's used:
auto separator = "";
for (const Person& person: people) {
    std::cout << separator;
    separator = "\n\n";

    person.printInfo();
}

A nice touch would be to allow printing to somewhere other than std::cout, by passing the stream as an argument to printInfo():
void printInfo(std::ostream& out = std::cout) const {
    out << "Name: " << name << '\n'
        << "Age: " << age << '\n';
}

Modified code
I've rearranged the headers a bit, to ease the process for when you want to separate the class definition from the test program.  I've changed the file read/write to work purely in memory for the test (I don't like tests that leave files lying about; using real files also increases fragility of the tests).  And I've added some validation of ages and names.
#include <algorithm>
#include <istream>
#include <ostream>
#include <stdexcept>
#include <string>

class Person
{
private:
    int age;
    std::string name;

public:
    void setAge(int age) {
        if (age < 0) {
            throw std::range_error("Negative age");
        }
        this->age = age;
    }

    int getAge() const {
        return age;
    }

    void setName(std::string name) {
        auto first_nonspace
            = std::find_if_not(name.begin(), name.end(),
                               [](unsigned char c){ return std::isspace(c); });
        if (first_nonspace == name.end()) {
            throw std::range_error("Invalid name");
        }
        this->name = name;
    }

    std::string getName() const {
        return name;
    }

    void printInfo(std::ostream& out) const {
        out << "Name: " << name << '\n'
            << "Age: " << age << '\n';
    }
};

std::istream& operator>>(std::istream& is, Person& person)
{
    int age;
    is >> age;

    std::string name;
    std::getline(is, name);

    if (is) {
        person.setAge(age);
        person.setName(name);
    }

    return is;
}

std::ostream& operator<<(std::ostream& os, const Person& person)
{
    return os << person.getAge() << ' '
              << person.getName() << '\n';
}

#include <iostream>
#include <sstream>
#include <vector>

int main()
{
    std::ostringstream myfile;
    myfile << "3 Baby\n";
    myfile << "48 Linus Torvalds\n";
    myfile << "62 Bill Gates\n";
    myfile << "115 Kane Tanaka\n";

    std::istringstream mysamefile(myfile.str());

    std::vector<Person> people;
    Person person;
    while (mysamefile >> person) {
        people.push_back(std::move(person));
    }

    auto separator = "";
    for (const Person& person: people) {
        std::cout << separator;
        separator = "\n\n";

        person.printInfo(std::cout);
    }
}


Maximal Char Requests in a String I have been trying to deal with this problem set, and I have figured out how to solve it as well. But there is one problem that is Timeout error for some hidden cases, but no wrong answers

PROBLEM STATEMENT

In this challenge, a string and a list of intervals are given. The string consists of English letters only and it can contain both lowercase and uppercase letters.

For two different letters, we say that the first letter is greater than the second letter when the first letter comes later in the alphabet than the second letter ignoring the case of the letters. For example, the letter 'Z' and 't' are greater than the letters 'b' and 'G', while the letters 'B' andd 'b' are equal as case is not considered.

The task is the following. For each given interval, you need to find the count of the greatest letter occurring in the string in that interval, ignoring the case of the letters, so occurrences of, for example, a and A are occurrences of the same letter.

Consider, for example, for the string "AbaBacD". In the interval, [0, 4], the greatest letter is 'b' with count 2.

Input Format

The first line contains integer N, denoting the length of the input string.

The second line contains string S.

The third line contains an integer Q, denoting the number of intervals. Each line of the  Q subsequent lines contains two space-separated integers xi and yi, denoting the beginning and the end of ith interval.

Output Format

For each interval, print the count of the greatest letter occurring in the string in that interval.

My Algorithm

1. To lowercase the string
2. Loop through the queries array
3. Get the string from start interval to end interval
4. Find greatest char in the string
5. Count the occurrence, and add it to the array
6. Return the array

My Code

def getMaxCharCount(s, queries):
    # queries is a n x 2 array where queries[i][0] and queries[i][1] represents x[i] and y[i] for the ith query.
    maxCount = []
    finalWord = s.lower()
    for interval in queries:
        if interval[0] == interval[1]:
            maxCount.append(1)
        else:
            string = finalWord[interval[0]:interval[1]+1]
            maxCount.append(string.count(max(string)))
    return maxCount

Any help would be appreciated

PEP 8

The Style Guide for Python Code’s naming conventions recommends again mixedCase identifiers.  CapWords are for classes and types, snake_case is for functions, methods and variables.  Thus, getMaxCharCount, maxCount and finalWord should be renamed to get_max_char_count, max_count and final_word.

string

string is an importable module in Python.  Its use as an identifier should be discouraged.

Intervals

interval is a perfectly fine variable, but interval[0] and interval[1] are confusing.  Are those the zeroth and first intervals in a list of intervals?  It would be better to unpack the interval immediately into the starting and ending values of the interval.  This can even be done directly by the for statement:

    for start, end in queries:
        if start == end:
            ...

Special Casing

You have special-cased the length 1 interval.  Why?  Does the general purpose version of the code not work for the length one interval?  Is it too slow?  Is the special-case a common occurrence, or does checking for it actually slow down the general purpose case be introducing an unnecessary if statement?

List Comprehension

If we remove the special-case, your code reads approximately:

    max_count = []
    for start, end in queries:
        max_count.append(...)

This pattern is usually re-written using list-comprehension, since it is much faster than repeated calls to .append():

    max_count = [ ... for start, end in queries ]

The only issue is what should be used in place of ....  Prior to Python 3.7, there was no easy way to refer to the result of a computation multiple times in one expression.  Python 3.8 introduces the “walrus operator” (:=) which allows variables to be assigned inside of other expressions.  We can use this to avoid referencing and slicing final_word twice:

    max_count = [ (span := final_word[start:end+1]).count(max(span)) for start, end in queries ]

Reworked Code (Python 3.8)

def get_max_char_count(s, queries):
    final_word = s.lower()
    return [(span := final_word[start:end+1]).count(max(span)) for start, end in queries]

This code may be slightly faster than your original code, with the speed up mostly from replacing .append with list comprehension.  To really see significant speed gains, you need to improve the algorithm.

Algorithmic Improvement

Since this is a coding challenge, I won’t give you code for the better algorithm, but I will get you started in the right direction.

How many times are you looking at any given character of final_word?

Consider the following test case:

2000
AAAAAAAAAAA...[2000 A’s] ... AAAAAAAAAAAA
2000
0 1999
0 1999
0 1999
  :      :
[2000 duplicate lines]
  :      :
0 1999
0 1999

The result should be a list of 2000 copies of the number 2000.  We did this in our head.  Your algorithm will test each character 4000 times; 2000 times during max and another 2000 times during count.  Clearly, we’ve got room for improvement.  The ideal would be if each character was only tested twice, once during max, and once during count.  But how can we achieve this?

Consider another test case:

       20
     AAAABBBBCCCCBBBBCCCC
     3
     0 11
     4 15
     8 19

Can you see any way of computing intermediate results and reusing them?

   Can you divide this into 5 subproblems?  Can you combine the first 3 subproblems together to get the first answer?  4 A’s, 4 B’s and 4 C’s makes 4 C’s.  Subproblems 2 to 4 to get the second answer?  4 B’s, 4 C’s and 4 B’s makes 4 C’s.  Subproblems 3 to 5 to get the 3rd answer?  4 C’s, 4 B’s and 4 C’s makes 8 C’s.  How would you store the subproblems results?  Where did we get the boundaries of the subproblems?


Insertion Sort in Python This is what I wrote for insertion sort in python. But somehow I'm feeling that it's between bubble and insertion sort.

def insertionSort(lst):
    for i in range(1, len(lst)):
        if lst[i] < lst[i-1]:
            for j in range(i):
                if lst[i] < lst[j]:
                    lst[i], lst[j] = lst[j], lst[i]
    return lst

if __name__ == '__main__':
    lst = [64, 88, 51, 65, 90, 75, 34, 79, 46, 36]
    print insertionSort(lst)

Your solution looks ok to me. But let's understand better how this sort method actually works.

The insertion sort, works as it follows: always maintains a sorted sublist in the lower positions of the list. Each new item is then "inserted" back into the previous sublist such that the sorted sublist is one item larger. See below how insertion sorting process works. The shaded items represent the ordered sublists as the algorithm makes each pass.

(source: interactivepython.org)

We begin by assuming that a list with one item (position 00) is already sorted. On each pass, one for each item 1 through n−1, the current item is checked against those in the already sorted sublist. As we look back into the already sorted sublist, we shift those items that are greater to the right. When we reach a smaller item or the end of the sublist, the current item can be inserted.

(source: interactivepython.org)

Above, a sorted sublist of five items consisting of 17, 26, 54, 77, and 93 exists. We want to insert 31 back into the already sorted items. The first comparison against 93 causes 93 to be shifted to the right. 77 and 54 are also shifted. When the item 26 is encountered, the shifting process stops and 31 is placed in the open position. Now we have a sorted sublist of six items.

With this being said, I'd go with this implementation:

def insertionSort(lst):
    for index in range(1, len(lst)):

        currentvalue = lst[index]
        position = index

        while position > 0 and lst[position - 1] > currentvalue:
            lst[position] = lst[position - 1]
            position = position - 1

        lst[position] = currentvalue

lst = [54, 26, 93, 17, 77, 31, 44, 55, 20]
insertionSort(lst)
print(lst)


Ruby html href parser working slow I wrote this simple HTML parser in Ruby.

Basically I have a folder that has files. Every file has crawled web pages. The only addition within these files is before the HTML content there is a page metadata, #URL is one of them.

What I am aiming is to have two files:

1)Will contain: currentURL, destinationURL, description

2)Will contain: currentURL, destinationURL, word, count

So if I did that for this current page and assuming that href that I am looking at is:

Stack Overflow

1st file would have

codereview,stackexchange.com, http://stackoverflow.com, Stack Overflow

2nd file would have

codereview,stackexchange.com, http://stackoverflow.com, Stack, 1

codereview,stackexchange.com, http://stackoverflow.com, Overflow, 2

However my code works really slow. It has been longer than an hour and still parsing a file with size of 31MB.

My code is:

beginning_time = Time.now

folder = "allParts/"
extension = "*.dat"
urlMet = "#URL:"
newLine = "\n"
files = Dir[folder+extension]
hrefTag = ""
hrefEnd = ""
firstOutput = "firstOut.txt"
secondOutput = "secondOut.txt"

File.open(firstOutput, 'w') {}
File.open(secondOutput, 'w') {}

countFile = 1

for file in files do
    puts file + "#: "+countFile.to_s
    countFile++
    File.open(file, "r") do |f|
        source = ""
        f.each_line do |line|
            dest = ""
            desc = ""
            if line.include? urlMet
                source = line[/#{urlMet}(.*?)#{newLine}/m, 1]
            end
            if line.include? hrefTag
                dest = line[/#{hrefTag}(.*?)#{qtMark}/m, 1]
                descStIn = line.rindex(descStart)
                descEndIn = line.rindex(hrefEnd)
                desc = line[descStIn+2..descEndIn-1]
            end
            if (source != "" && dest != "")
                occur = Hash.new(0)
                mainEntry = "original-url=\"" + source +
                "\", dest-url=\"" + dest + "\""
                descEntry = ""
                if (desc != nil && desc != "")
                    descEntry = ", desc=\"" + desc + "\""
                    words = desc.split(' ')
                    words.each { |word| occur[word] += 1 }
                end
                firstEntry = mainEntry+descEntry+"\n\n"
                File.open(firstOutput, 'a') { |file|
                    file.write(firstEntry)
                }
                occur.each { |word, occurrance|
                    wordEntry = ", word=\"" + word +
                    "\", count=" + occurrance.to_s
                    secondEntry = mainEntry+wordEntry+"\n\n"
                    File.open(secondOutput, 'a') { |file|
                        file.write(secondEntry)
                    }
                }
            end
      end
    end
end

end_time = Time.now
puts "Time elapsed #{(end_time - beginning_time)} seconds"

You are trying to parse HTML with regexes, which is widely considered to be a bad idea. For example, all these tags would have equivalent link text and target URL:

Link Text

Link Text

   Link Text

Link Text

Link Text

A line-based parser will fail here, and a regex-based parser will be very difficult to write correctly. So please use an existing HTML parser. The result will be fast and elegant, but as I don't use Ruby I can't give examples.

Let's move on to coding style. You are using multiple naming styles: beginning_time and newLine. Whatever you do, be consistent, although I'd recommend snake_case for variable names.

Some of your variable names contain abbreviations where none are necessary: quote_character is probably better than qtMark, descStart would probably be better described as description_start.

You used many variables to make your regexes appear more modular, but they are not. Your regexes will become more readable if you write them down directly rather than assembling them. You could also specify them outside of your loops:

href_regex = /href="([^"]+)"/

Points to note here:

[^"]+ is generally preferable over .*?
The /m option was highly unnecessary, because you process your input line by line. So there is no way that your . could ever match a newline in your original regex.

In your regexes, you used #{…} interpolation a lot. Strangely you didn't do any of that when assembling other strings, instead using less readable concatenation. E.g. we could rewrite

mainEntry = "original-url=\"" + source +
"\", dest-url=\"" + dest + "\""

as

main_entry = "original-url=\"#{source}\", dest-url=\"#{dest}\""

This is still pretty unreadable because of those pesky escapes – using the alternate %Q() literal syntax we could simplify this to:

main_entry = %Q(original-url="#{source}", dest-url="#{dest}")

… and ditto in the similar cases.

A snippet like

words = desc.split(' ')
words.each { |word| occur[word] += 1 }

could be improved in two ways:

get rid of the unnecessary variables. The code is already sufficiently self-documenting with the word parameter to the block
rename occur to something sensible like word_count

You declare your source variable outside of the each_line loop. It would be better to declare your variables as close to their point of usage as possible. Furthermore, you initialize them with the empty string. I (as a Ruby newbie) would probably initialize them to nil instead. This has the further advantage that we can remove those line.include? "some string" tests which do not even guarantee that the regex will match. The  String#[Regex, capture] method returns either the string matched by that capture group, or nil – which is exactly what we want.

Don't re-open a file all the time inside a tight loop, this is quite expensive. Open the file once, write to it multiple times. This will probably remove your main performance bottleneck.

If we stuff all this advice into your code, we might end up with the following refactoring:

start_time = Time.now

file_count = 1

File.open("firstOut.txt", 'w') do |out1|
    File.open("secondOut.txt", 'w') do |out2|
        for file_name in Dir["allParts/*.dat"] do
            puts "#{file_name}#:#{file_count}"
            file_count++

            File.open(file_name, "r") do |file|
                file.each_line do |line|
                    source      = line[/#URL:(.*?)\n/, 1]
                    destination = line[/')   + 2
                        description_end   = line.rindex('') - 1
                        description = line[description_start .. description_end]
                        if (description != nil && description != "")
                            out1.write(%Q(#{main_entry}, desc="#{desc}"\n\n))
                            description.split(' ').each do |word|
                                word_count[word] += 1
                            end
                        else
                            out1.write(%Q(#{main_entry}\n\n))
                        end

                        word_count.each do |word, count|
                            out2.write(%Q(#{main_entry}, word="#{word}", count=#{count}\n\n))
                        end
                    end
                end
            end
        end
    end
end

puts "Time elapsed #{Time.now - start_time} seconds"

Note that this can be improved further – the high level of indentation suggests we should extract some parts as separate functions. I further want to reiterate that I'm not a proficient Rubyist, so there might be some egregious oversights in the above code.


Multiply vector elements by a scalar value using STL and templates I wrote a small scientific simulation where I wanted to manipulate vectors with the same kind of functionality that Python's NumPy vectors use. Namely, I wanted to be able to add vectors together, multiply by constants, etc. I have need for both vectors of type int and floating point, so I wanted to template the overloading. As an example (and for something to review), I overloaded scalar multiplication as:

template <class T, class Q>
vector <T> operator*(const Q c, const vector<T> &A) {
  vector <T> R(A.size());
  for(int i=0;i<A.size();++i)
    R[i] = c*A[i];
  return R;
}

The STL algorithm way I hinted at looks like this:

template <class T, class Q>
vector <T> operator*(const Q c, const vector<T> &A) {

  vector <T> R(A.size());
  std::transform(A.begin(), A.end(), R.begin(),
                 std::bind1st(std::multiplies<T>(),c));
  return R;
}

I would appreciate all criticism relevant to code, style, flow. My project is in C++11, so in theory I could use the newer range-based for loops for readability (I do in other parts of my code). Additionally, I think I could rewrite this using std::transform, but I'm not sure if it would help improve the efficiency or the readability of my code.

I would use the second version that you posted. Here are some problems with the first version:

1. Prefer passing by value rather than making an explicit copy.

This is a problem in both versions of your code. You pass A via constant reference. Normally this is a good thing, but then you then make a copy of A explicitly. In this case, it is possible to miss out on a compiler optimization called copy elision (second reference here). Passing by value would not cause any variable aliasing problems.

Update: Imagine somebody (maybe you) using your function.

std::vector <int> v1 = // ... insert elements
auto v2 = 3 * v1 ;

For simplicity, let's say variable v2 is located at address 0x10.
When you do 3 * v1, you are calling your function.

R would then be created at let's say address 0x20. When your function returns, the contents of R would have to be copied from 0x20 to v2 at 0x10 and then the destructor for R would have to be called. Without copy elision, this would require copying every element from R into v2.

Now in C++11, there is something called move semantics. This means, that without copy elision, R would not have to copy every element into v2, it could swap pointers. This is much faster, but it is still slower than copy elision.

With copy elision, instead of creating R at 0x20, it uses address 0x10 and builds v2 in place. With copy elision, nothing has to be copied and no pointers have to be swapped because v2 is built in place.

Here's how I would re-write the function:

template <class T, class Q>
std::vector <T> operator* (const Q c, std::vector <T> A)
{
    std::transform (A.begin (), A.end (), A.begin (),
                 std::bind1st (std::multiplies <T> () , c)) ;
    return A ;
}

2. If you have to use an index-loop for some reason, rather than a range-loop or iterators, make sure you are comparing compatible types.

You could do something like this:

typedef std::vector <T>::size_type size_type ;
for (size_type i = 0; i < A.size (); ++i)

C++11 has expanded the functionality of the using keyword. So instead of a typedef, you could also do:

using size_type = decltype (A.size ()) ;

Update: The reason you want to use compatible types is because overflowing a signed number is undefined behavior in C++. This means anything can happen. Consider this example:

const unsigned size = std::numeric_limits <int>::max () + 5 ;
std::vector <char> v1 (size) ;

for (int n = std::numeric_limits <int>::max (); n < v1.size (); ++n) {
    std::cout << n << ',' << v1.size () << '\n' ;
}

Different compilers on different platforms will do different things with the code above because there is undefined behavior (an int can never reach std::numeric_limits <int>::max () + 5).

3. Consider wrapping the std::vector into a class.

Programs typically use std::vector for many things. Overloading a global operator like you do would make me uncomfortable using your code.

4. Obligatory avoid using namespace std;.

5. Use more meaningful variable names and typenames.

Unless A, R, Q, etc are commonly understood names for special variables in your scientific community, use more meaningful names.

6. Do not omit braces for if-statements or loops.

This is more a subjective style guideline than anything.


Tough List Comprehension Split Letters and Numbers So, another challenge from my professor. I've finished it, but wanted to get some feedback, and see how other people might work this problem.

  Assume you have a string, such as "Hello 34215 World 5620 From 384 Bloomington"

  Use list comprehension to separate the words from the numbers, and the numbers from the words. Then order the numbers from smallest to largest. Print out these words and numbers separated by a "|". The output for the example given above would be:

>>>
Hello World From Bloomington | 012233445568

The challenge is to write the code in two lines, one of the lines being the definition of the string.

This is what I wrote:

string = "Hello 34215 World 5620 From 384 Bloomington"
print str(" | ".join(["".join([letter for letter in string if letter.isalpha() or letter.isspace()]), "".join(sorted([num for num in string if num.isdigit()]))])).replace("  ", " ")

It works! The output is identical to the output displayed. But, I'd love to hear feedback about how this is done, or if there are any pieces of redundant code, etc. I don't use lambda, map, set, etc. nor do I import any libraries, but if you find that there is a way to do it with any of those pieces, feel free to share!

Padraic Cunningham nailed the shortest form in his comment:

" ".join(x for x in s.split() if not x.isdigit()) + " | " + "".join(sorted(x for x in s if x.isdigit()))

Explanation:

x for x in s.split() if not x.isdigit() -- the list of words that don't have digits. Note that the split method without parameters uses space as the separator.
x for x in s if x.isdigit() -- the list of digits extracted from the string, unsorted
From the above 2 it's easy to combine to get the desired result.

Although this is short, it checks every letter twice, first using not x.isdigit() to get only the letters and then x.isdigit() to get only the digits. We could improve that by using a helper function to partition the list of words on the outcome of x.isdigit(), like this:

def partition(pred, iterable):
    trues = []
    falses = []
    for item in iterable:
        if pred(item):
            trues.append(item)
        else:
            falses.append(item)
    return trues, falses

nums, words = partition(str.isdigit, s.split())
' '.join(words) + ' | ' + ''.join(sorted(''.join(nums)))

Of course, this is much more than one line...


Distinguishing user input numbers and characters I am currently taking a computer science class in high school, and it is my first time programming (2 weeks since I started). I have currently finished my program; however, there were a few details that bothered me. I was supposed to create a program which can calculate average of individual marks and then calculate all of the average marks previously entered by user.

However, I noticed something that bothered me, which was the fact that if I were to enter a number when asked to enter a name, it would assume that number is a name. Also, when I asked for the 5 marks, the program would run fine until an error occurs if I accidentally type in a letter or word. I have done some research and found system scanners; however, I still don't understand the concept in a looping situation.

import java.io.*;
public class loopingEx6Final {
    public static void main(String args[]) throws IOException{
        BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        double average, totalaverage = 0;
        int Mark, Marktotal=0,  marktotalsum=0, count=0;
        String strName, strMark;
        System.out.println("This program will calculate an individual's personal average and then calculate the class average whn instructed.");
        System.out.println("Please type (finish) in order to calculate class average.");
        System.out.println("Please enter a name.");
        strName = br.readLine ();

        while (!strName.equals("finish")){
            System.out.println("The follwing will calculate the average five marks of "+ strName +".");
            System.out.println("Please enter 5 marks.");
            count++;
            for  (int i=0;i<5;i++) {
                Mark = Integer.parseInt(br.readLine());
                    Marktotal= Marktotal + Mark;

            }
            average = Marktotal/5;
            System.out.println(strName+"'s average is "+average);
            Marktotal= 0;
            System.out.println("\n"+"Please enter a name.");
            strName = br.readLine ();
            totalaverage=(totalaverage+average)/count;
        }
        System.out.println("The class average of all input grades is:");
        System.out.println(totalaverage);
        System.out.println();
        System.out.println("Thank you for using the program.");
    }
}

Variables in java are camelCase, it's odd that you follow it in cases like strName but  ignore it with MarkTotal or totalaverage. No matter what style you choose, be consistent about it, for instance I like here how the latter variable has an equals sign between it and the 0, yet everything's together in the proceeding line. Try to maintain a space between your variables, and declare only 1 variable per line. Additionally, make it a habit to define variables near where they're actually used. Narrow your scope as much as possible as it will help in future maintainability.

These may seem like nitpicks for now, but down the line and as your programs grow they will make things easier to locate, read, and understand.

You don't need to repeatedly call System.out.println, you can use \n to add a new line where necessary. Your several calls in the beginning can be something like this:

This line:

 Marktotal= Marktotal + Mark;

Can be shortened to MarkTotal += Mark; but minding the tips I said earlier it would be more markSum += mark;

Lastly you have some variables that are only used once, when that's the case you can usually get rid of them and simply replace them with the method call.

Here's your implementation with these suggestions:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;

public class ForNing {
    public static void main(String args[]) throws IOException {
        BufferedReader input = new BufferedReader(
            new InputStreamReader(System.in)
        );

        System.out.print(
            "This program will calculate an individual's personal average" +
            " and then calculate the class average when instructed. " +
            "\nEnter a name: "
        );

        String studentName = input.readLine();
        int studentCount = 0;
        double classAverage = 0;

        while (!studentName.equals("finish")){
            System.out.println(
                "The follwing will calculate the average five marks of "+
                studentName +".\nPlease enter 5 marks."
            );

            studentCount++;

            int markSum = 0;
            for (int i = 1; i <= 5; i++) {
                markSum += Integer.parseInt(input.readLine());
            }

            double average = markSum / 5;
            markSum = 0;
            classAverage += average;

            System.out.println(
                studentName + "'s average is: " + average +
                ".\n\nPlease enter the next student's name " +
                "or enter 'finish' to calculate the class average."
            );

            studentName = input.readLine();
        }

        classAverage /= studentCount;

        System.out.println(
            "The class average of all input grades is: " + classAverage +
            ".\n\nThank you for using the program."
        );
    }
}


Improving readability and logic on "re-indexing" method This is a method which takes an input of a "page" object, which includes SortOrder.  It then fetches the existing page items from a database and attempts to re-index.

I don't think this is at all efficient and reads horribly and wondered if anybody could make some recommendations?

    public Boolean ReIndexOutputTemplatePages(OutputTemplatePage page)
    {
        try
        {

            int? requiredIndex = page.SortOrder;

            var items = from t1 in db.OutputTemplatePages
                        where (t1.OutputTemplateId == page.OutputTemplateId)
                        orderby t1.SortOrder
                        select t1;

            Boolean boolHasBeenAdded = false;
            int ctr = 0;
            if (items.Count() == 0)
            {
                Add(page);
                boolHasBeenAdded = true;
            }
            else
            {
                foreach (OutputTemplatePage template in items)
                {
                    if (requiredIndex == ctr)
                    {
                        Add(page);
                        ctr++;
                        boolHasBeenAdded = true;
                    }

                    OutputTemplateRepository repo = new OutputTemplateRepository();
                    OutputTemplatePage pg = repo.GetOutputTemplatePageById(template.Id);
                    pg.SortOrder = ctr;
                    repo.Save();

                    ctr++;

                }
            }
            if (!boolHasBeenAdded)
            {
                Add(page);
            }
            return true;
        }
        catch (Exception)
        {
            return false;
        }
    }

I have the following comments and recommendations:

The try/catch is too general: if anything goes wrong anywhere, you return false. This may mask errors. Catch more specific exceptions and narrow the scope of your try/catch to where you are okay with things going wrong. (If you don't EXPECT anything to ever go wrong in certain parts of the code, you probably want to "fail fast" and catch that exception at a higher level. This could hide lots of kinds of errors during dev/test.)
Name mismatch: requiredIndex does not match .SortOrder (I'm not saying they need to be same exact name, but these seem to describe different concepts to me)
Does .SortOrder need to be optional? Consider enforcing that it be set by changing the orginal OutputTemplatePage class to require it. Is it an enum? If so, perhaps the default value could be Default (as in "use the default sort order")
Name choice: Boolean boolHasBeenAdded might be better as Boolean pageHasBeenAdded (prefix notation has been out of vogue for years - since .NET, certainly). Also, I usually prefer language types (bool) to the .NET types, but that's just style/preference.
This block could go away if you invert the next conditional (since this block happens anyway at the end):

    if (items.Count() == 0)
    {
        Add(page);
        boolHasBeenAdded = true;
    }
    else

... to ...

        if (items.Count() != 0)

Consider moving the following code into a well-named helper method to make its intent more apparent:

            OutputTemplateRepository repo = new OutputTemplateRepository();
            OutputTemplatePage pg = repo.GetOutputTemplatePageById(template.Id);
            pg.SortOrder = ctr;
            repo.Save();

It is unclear to me what repo.Save() does - seems like it might have side-effects. All the more reason for prior suggestion.
Rename ctr to something more descriptive. Is it the sort index? Of course, setting SortOrder = ctr makes me wonder if SortOrder is named correctly. "Sort order" makes me thing of high level approach to sorting - like ascending vs. descending? Not clear. Makes it hard to grok.

[Edit] Actually, this largest problem might be that this code is not in the right class which might be the source of some awkwardness. Consider making ReIndexOutputTemplatePages a method on OutputTemplateRepository to clean up the code leading up to rep.Save.

Hope that helps!


Generating the sequence of the digits that appear in the sequence of natural numbers This is a revisit to a question already asked here about a year and a half ago.

Project Euler's Problem 40 involves generating the sequence of digits that appear in the natural numbers, all concatenated together (123456789101112131415161718192021…).  As part of the solution, I wrote this:

def counting_digits():
    for number in count(1):
        for digit in str(number):
            yield digit

After finishing the solution, I tried this:

def counting_digits():
    return (digit for number in count(1) for digit in str(number))

My questions:

In the face of Python 3, Which solution is more pythonic?
As someone who just wants to understand the code. Which is easier/clearer to read?
If you haven't read the statement for Project Euler's Problem 40, Which version lets you better know what the function does?

Performance is not part of the question: pythonism and understandability is the issue.

The one where you return a generator is easier to read were it the case that there was only one for. Also, as OP pointed out, the associativity of nested for loops is not what you'd expect in Python.

The top one may be slightly easier to read for some people; it would be MUCH easier to read if there were comments or if the variables were semantically named, and the indented structure allows for comments much better. With semantically-named variables, the first version is easier to read; I'd only use single-letter variable names as the pattern in list comprehensions.


Library to verify GTINs of various lengths I'm writing a library to verify GTINs (GTIN-8, GTIN-12, GTIN-13, GTIN-14) and calculate their check digits. The calculation of the check digits don't differ from type to type, so I can use the same code for all types of GTINs.

My question: Should I use separate classes for each type of GTIN or should I store the length into an instance attribute?

I have listed two options (simplified):

Option #1

import re

class GTIN:
    def __init__(self, gtin):
        self.gtin = self.parse(gtin)

    @classmethod
    def parse(cls, gtin):
        parsed_gtin = cls.sanitize(gtin)
        if not cls.check_format(parsed_gtin)
            return None
        return parsed_gtin

    @classmethod
    def sanitize(cls, gtin):
        return re.sub(r'[\s\-]+', '', gtin)

    @classmethod
    def check_format(cls, gtin):
        if re.match(cls.FORMAT, gtin):
            return True
        return False

class GTIN8(GTIN):
    FORMAT = r'^[0-9]{7}[0-9?]{1}$'

class GTIN12(GTIN):
    FORMAT = r'^[0-9]{11}[0-9?]{1}$'

The given GTIN string can contain a ? if the check digit should be calculated.

Option #2

import re

class GTIN:
    ALLOWED_LENGTHS = [8, 12, 13, 14]

    def __init__(self, gtin):
        self.gtin = self.parse(gtin)

    @classmethod
    def parse(cls, gtin):
        parsed_gtin = cls.sanitize(gtin)
        if not cls.check_format(parsed_gtin)
            return None
        return parsed_gtin

    @classmethod
    def sanitize(cls, gtin):
        return re.sub(r'[\s\-]+', '', gtin)

    @classmethod
    def check_format(cls, gtin):
        if len(gtin) in cls.ALLOWED_LENGTHS:
            self.length = len(gtin)
        else:
            return False

        format = r'^[0-9]{{{}}}[0-9?]{{1}}$'.format(self.length - 1)
        if re.match(format, gtin):
            return True
        return False

I assume the variety of GTIN's are a bounded set.  If so, I see no use
in the inheritance.  Go with Option #2. And on that option I would recommend a
few changes

Pre-build your formats into a dict based on length:

This pre-builds and compiles the format regex's

formats = {l: re.compile(r'^[0-9]{{{}}}[0-9?]{{1}}$'.format(l - 1))
           for l in (8, 12, 13, 14)}

Add a __str__ method for easier debugging:

This makes it easier to see what is going on in the debugger.

def __str__(self):
    return self.gtin if self.gtin is not None else 'invalid'

If you have an early return, put it in the if, not else:

I would generally code something like this:

if len(gtin) in cls.ALLOWED_LENGTHS:
    self.length = len(gtin)
else:
    return False

as this:

if len(gtin) not in cls.formats:
    return False
....

Define a __len__ method if needed:

I removed the self.length as it was not used internally, and a better way to publish the length is with a __len__.

def __len__(self):
    return len(self.gtin) if self.gtin is not None else 0

Consume bool evaluations directly:

If you need a bool and are performing a test to determine the bool state, you rarely need an if.  I redid this:

if re.match(format, gtin):
    return True
return False

as this (with some other changes also incorporated):

return cls.formats[len(gtin)].match(gtin) is not None

So there is no reason for the if, the result of the test can be returned (or assigned to a variable) directly.

Entire Class:

import re

class GTIN:
    formats = {l: re.compile(r'^[0-9]{{{}}}[0-9?]{{1}}$'.format(l - 1))
               for l in (8, 12, 13, 14)}

    def __init__(self, gtin):
        self.gtin = self.parse(gtin)

    def __str__(self):
        return self.gtin if self.gtin is not None else 'invalid'

    def __len__(self):
        return len(self.gtin) if self.gtin is not None else 0

    @classmethod
    def parse(cls, gtin):
        parsed_gtin = cls.sanitize(gtin)
        if not cls.check_format(parsed_gtin):
            return None
        return parsed_gtin

    @classmethod
    def sanitize(cls, gtin):
        return re.sub(r'[\s\-]+', '', gtin)

    @classmethod
    def check_format(cls, gtin):
        if len(gtin) not in cls.formats:
            return False
        return cls.formats[len(gtin)].match(gtin) is not None


DAL mapping efficiency I am wondering if I can make these classes a bit more efficient.

Test Results

Single Run

Method 1: 5 Columns - Text Query - 81178 Records = 00:00:00.6390366
secs
Method 2: 5 Columns - Text Query - 81178 Records = 00:00:00.5360307
secs

10 Run Loop

Method 1: 5 Columns - Text Query - 81178 Records = 00:00:05.3253045
secs
Method 2: 5 Columns - Text Query - 81178 Records = 00:00:05.0912912
secs

100 Run Loop

Method 1: 5 Columns - Text Query - 81178 Records = 00:00:54.1270959
secs
Method 2: 5 Columns - Text Query - 81178 Records = 00:00:53.8710813
secs

All 3 attempts for both methods never peak over 25% CPU usage.

As you can see there really is no significant improvement over either method, and method 2 (judging by CPU usage) does not seem to multi-thread.

I am thinking that if I can get rid of my usage of reflection to map the columns to strongly-typed classes that it would make a significant boost to both methods performance, and I am sure that I can make improvements to the asyncronicity of method 2 as well... I just don't know how.

WrapperTest.cs

    private static IList<T> Map<T>(DbDataReader dr) where T : new()
    {
        try
        {
            // initialize our returnable list
            List<T> list = new List<T>();
            // fire up the lamda mapping
            var converter = new Converter<T>();
            // read in each row, and properly map it to our T object
            var obj = converter.CreateItemFromRow(dr);

            // reutrn it
            return list;
        }
        catch (Exception ex)
        {
            // Catch an exception if any, an write it out to our logging mechanism, in addition to adding it our returnable message property
            _Msg += "Wrapper.Map Exception: " + ex.Message;
            ErrorReporting.WriteEm.WriteItem(ex, "o7th.Class.Library.Data.Wrapper.Map", _Msg);
            // make sure this method returns a default List
            return default(List<T>);
        }
    }

This is a continuation of this question.

The code above definitely runs more efficiently now.  Is there any way to make it better?

Use LINQ expression compilation to generate mapping code at runtime. The concept is to generate a method that does obj.Property1 = dataReader["Property1"]; ... dynamically.

public class Converter<T> where T : new()
{
    private static ConcurrentDictionary<Type, object> _convertActionMap = new ConcurrentDictionary<Type, object>();
    private Action<IDataReader, T> _convertAction;

    private static Action<IDataReader, T> GetMapFunc()
    {
        var exps = new List<Expression>();
        var paramExp = Expression.Parameter(typeof(IDataReader), "dataReader");
        var targetExp = Expression.Parameter(typeof(T), "target");
        var getPropInfo = typeof(IDataRecord).GetProperty("Item", new[] { typeof(string) });
        foreach (var property in typeof(T).GetProperties())
        {
            var getPropExp = Expression.MakeIndex(paramExp, getPropInfo, new[] { Expression.Constant(property.Name, typeof(string)) });
            var castExp = Expression.TypeAs(getPropExp, property.PropertyType);
            //var bindExp = Expression.Bind(property, castExp);
            var bindExp = Expression.Assign(Expression.Property(targetExp, property), castExp);
            exps.Add(bindExp);
        }
        return Expression.Lambda<Action<IDataReader, T>>(Expression.Block(exps), new[] { paramExp, targetExp }).Compile();
    }

    public Converter()
    {
        _convertAction = (Action<IDataReader, T>)_convertActionMap.GetOrAdd(typeof(T), (t) => GetMapFunc());
    }

    public T CreateItemFromRow(IDataReader dataReader)
    {
        T result = new T();
        _convertAction(dataReader, result);
        return result;
    }
}

Test method with 80,000 x 100 iteration

    static void Main(string[] args)
{
    var dummyReader = new DummyDataReader();
    var properties = typeof(DummyObject).GetProperties();
    var startDate = DateTime.Now;
    var converter = new Converter<DummyObject>();
    for (int i = 0; i < 80000 * 100; i++)
    {
        //var obj = CreateItemFromRow2<DummyObject>(new DummyDataReader());
        var obj = CreateItemFromRow<DummyObject>(dummyReader, properties);
        //var obj = converter.CreateItemFromRow(dummyReader);
        dummyReader.DummyTail = i;
    }

    //var obj = CreateItemFromRow2<DummyObject>(new DummyDataReader());
    Console.WriteLine("Time used : " + (DateTime.Now - startDate).ToString());
    Console.ReadLine();
}

Result:

CreateItemFromRow : 18.5 seconds
Converter<T> : 7.3 seconds

Map function:

    private static IList<T> Map<T>(DbDataReader dr) where T : new()
    {
            // initialize our returnable list
            List<T> list = new List<T>();
            // fire up the lamda mapping
            var converter = new Converter<T>();

            while (dr.Read()) {
                // read in each row, and properly map it to our T object
                var obj = converter.CreateItemFromRow(dr);
                list.Add(obj);
            }

            // reutrn it
            return list;
     }


Use weather API to get information in JavaScript + jQuery Please can any code experts review and improve my code?

HTML

<!DOCTYPE html>

    <title>API</title>
    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
    <link rel="stylesheet" href="//fonts.googleapis.com/css?family=font1|font2|etc" type="text/css">
    <link rel="stylesheet" type="text/css" href="style.css">

Description:

Temperature:

That's the HTML. It is just mostly tags with id to output info.

JavaScript

var ask = prompt("Type in your city or town that you want the weather for. Please make sure you write the first letter as capital letter and you spell it right.");
$.getJSON("https://api.openweathermap.org/data/2.5/weather?q="+ask+"&units=metric&appid=32b8cd17f2ff5d84d72342dd7408bab2", function(data) {
    console.log(data);
    var icon = "https://openweathermap.org/img/w/" + data.weather[0].icon + ".png";
    var weather = data.weather[0].main;
    var desc = data.weather[0].description;
    var temp = data.main.temp;
    var temp1 = temp + "℃"
    $("#icon").attr("src", icon);
    document.getElementById('weather').innerHTML = weather;
    document.getElementById('desc').innerHTML = desc;
    document.getElementById('temp').innerHTML = temp1;
});

If there is any other language than jQquery that I can use to get JSON files from API, please let me know because I find jQuery quite confusing.

Response to your question

  If there is any other language than Jquery that I can use to get json files from api, please let me know because i find Jquery quite confusing :-).

As was mentioned in a comment (as well as a link-only answer that has since been deleted), there are alternatives, including but not limited to:

vanilla JavaScipt:

with XMLHttpRequest - refer to Using XMLHttpRequest on MDN
with the fetch API

other libraries, some of which are listed on youmightnotneedjquery.com/:

reqwest
then-request
superagent

General review

User experience

You didn't state whether the specification was part of an assignment/task or just something you came up with, but the UX might be improved by having the prompt and Ajax request triggered by an event like a mouse click instead of page load.

If such a change was implemented, then it would be wise to cache the DOM references in variables instead of querying the DOM each time- e.g.

var weatherEl = document.getElementById('weather');
var descEl = document.getElementById('desc');
var tempEl = document.getElementById('temp');

//function to prompt

//AJAX response callback
function ajaxResponse(data) {
    //parse data
    //...
    weatherEl.innerHTML = data.weather[0].description;
    descEl = data.weather[0].description;
    tempEl.innerHTML = data.main.temp + "°C";
}

That way, the lines can be shorter, the DOM won't get queried each time, and you can likely eliminate those variables that are assigned and then only used once.

Perhaps a template system would help improve the process of updating the DOM elements instead of having to update each item individually.

Handling other responses and errors

The code above doesn't appear to (gracefully) handle any response other than a successful call to the API endpoint. When the user types in a city that the API doesn't recognize, then a 404 response is returned. If you continue to use the jQuery library, then a .fail() callback could be specified (see the $.getJSON() documentation for an example to handle that. Also, if a server error occurred, then a 5xx response might be returned.

Additionally, the code assumes that data.weather will be an array and have at least 1 element. What happens if either of those are not true? While it may seem implausible it may be possible and something to guard against.

Extra libraries

The code includes mousetrap.js and moment.js - I presume those correspond to the libraries Mousetrap and MomentJS, but it doesn't appear that those libraries are used by the code. Unless those are used by other code not included, those libraries can be removed to save the users time loading the page.


Reduce columns to produce product keycodes I have written the following code that achieves my desired goal of converting a "raw" CSV file into a keycoded list. The idea is that a person will get the first product they ask for, and English products are coded as blank because they are the default.

This gist contains a sample of the input and output data. As it stands, rubocop thinks my code is too complex, and I am inclined to agree with it here, as it feels like I am doing far more than I need to in order to get such a simple result.

Not sure if this is helpful but the rubocop stats I'm getting are as follows:

ABC size too high (19.44/15)
Cyclomatic complexity too high (9/6)
Method has too many lines (17/10) (is this one really even a problem?)
Perceived complexity too high (10/7)

Are any of these "true" problems? They seem to reflect my opinion that the code just turned out "ugly"

On paper it I narrowed it down to about 2 cases with some sub cases. (1. Normal header -> write 2.Header has "#" in it -> subcases) and in the second case it's either blank, "1", or a product name. If the line contains "English Map" I write nothing, otherwise I am to write the first product requested sequentially on the line.

require 'CSV'
in_file = 'test.csv'

def code_line_by_product(line)
  new_line = []
  tmp = nil
  e_line = false
  line.each do |item|
    if item[0].include?('#')
      if item[1] != ' ' && item[1] && item[1] != '1' && item[1] != ''
        e_line = true if item[1].include?('English')
        unless tmp || e_line
          tmp = item[1][0]
          new_line << tmp
        end
      end
    else
      new_line << item[1]
    end
  end
  new_line
end

CSV.open("#{File.basename(in_file, '.csv')}_coded.csv", 'a+') do |out|
    out << %w(First Last Company Address address2 City State Zip Country phone_number email_address keycode)
  CSV.foreach(in_file, headers: true) do |line|
    out << code_line_by_product(line)
  end
end

Also, I've been noticing that I use that construction of writing from one file to another a lot and was wondering if there was a more convenient way of doing it (such as writing a function that takes the in file, out file, and the desired transformation). While it's only 4 lines of code, I essentially use those nested block all the time and it would be nice to only have to write the transformation function and then just call a single function to execute it.

I would do something like this. It is untested and may have typos but it should give you some idea of how to break the logic up.  I would:

Express variable to what they mean
Use a class

Hope this helps

require 'CSV'

class CSVTranslator
  def self.write file_path, options={}
    new(file_path, options).translate
  end

  def initialize source_file, options={}
    @source_file = source_file
    @destination_file = options.fetch(:to) { "#{File.basename(source_file, '.csv')}_coded.csv" }
  end

  def translate
    CSV.open(destination_file, 'a+') do |out|
      out << headers
      each_row { |row| out << row }
    end
  end

  private

  attr_reader :source_file, :destination_file

  def headers
    %w(First Last Company Address address2 City State Zip Country phone_number email_address keycode)
  end

  def each_row
    CSV.foreach(source_file, headers: true) do |line|
      yield code_line_by_product(line)
    end
  end

  def code_line_by_product(line)
    new_line = {}
    english_language = false
    keycode = ""

    line.each do |header, value|
      if header.include?('#')
        english_language ||= value.include?('English')
        keycode = value.first if value.first != '1'
      else
        new_line[header] = value
      end
    end

    new_line['keycode'] = keycode unless english_language || keycode.blank?

    new_line
  end
end

CSVTranslator.write 'test.csv', to: 'test_coded.csv'


Calculating COUNT(*) of a MAX() + GROUP BY query I have a SELECT query I'm executing which has a MAX() column and a GROUP BY clause, and in addition to the results of this query that I need to return to the client, I also need to return the total count of all results as well.
Essentially my query is this:
SELECT id, col1, col2, MAX(col3) as col3
FROM tbl
GROUP BY col1, col2

It'll usually have a WHERE clause too.
When returning this data to the client, I also specify LIMIT and OFFSET clauses to limit the number of results being retrieved and displayed on the UI at one time. For a good UX, I also return and display the total number of results that the above SELECT query would produce if it didn't have the LIMIT and OFFSET clauses, so that the UI can display e.g. "41-60 of 6500 results".
Currently, I use a WITH temporary table to get what I want:
WITH temp AS (
    SELECT id, col1, col2, MAX(col3) as col3
    FROM tbl
    GROUP BY col1, col2
)
SELECT COUNT(*) FROM temp

But I'm concerned about the efficiency of this. The sans-LIMIT-and-OFFSET query could return hundreds of thousands of rows, so I'm thinking that the WITH approach to getting the total count isn't the best way to do this.
Is there a more efficient way, which I'm not thinking of? Either a more efficient way to get the COUNT I need or some thoughts on reusing a previous COUNT value?

Example data
Assume this is the data in my table:
id   col1  col2  col3
______________________
1     5     8     30
2     5     8     33
3     5     9     40
4     6     8     30
5     6     8     31
6     6     8     32
7     6     9     39
8     7     8     33
9     7     8     32
10    8     8     34

So my SELECT query would return this (assume the client specified LIMIT 4 OFFSET 0):
SELECT id, col1, col2, max(col3) as col3
FROM tbl
GROUP BY col1, col2
LIMIT 4
OFFSET 0;
    id   col1  col2  col3
    ______________________
    2     5     8     33
    3     5     9     40
    6     6     8     32
    7     6     9     39

And then I'd use that query without the LIMIT and OFFSET clauses as a subquery and SELECT COUNT(*) from it, which would return 6, and I'd return both the 6 and the results to the client. The client runs this query automatically every 10 or so seconds to keep the UI refreshed, and every time the client presses the arrows at the bottom of the table on the UI to go to the next page (i.e. specify a new OFFSET).

The exact schema of this stripped-down case is this:
CREATE TABLE test (id INT NOT NULL, col1 TEXT, col2 TEXT, col3 INT, PRIMARY KEY (id));

The only difference between my schema and this is that I also join on another table based on the id, and there are more than just 2 columns in the GROUP BY clause. I kept this example minimal because the WITH part is what's really important here.

EXPLAIN reports that it's indeed creating a temporary table of all 10 rows, hence my concern:
> EXPLAIN WITH temp AS ( SELECT id, col1, col2, MAX(col3) AS col3 FROM test GROUP BY col1, col2 ) SELECT COUNT(*) FROM temp;
+------+-------------+------------+------+---------------+------+---------+------+------+---------------------------------+
| id   | select_type | table      | type | possible_keys | key  | key_len | ref  | rows | Extra                           |
+------+-------------+------------+------+---------------+------+---------+------+------+---------------------------------+
|    1 | PRIMARY     | <derived2> | ALL  | NULL          | NULL | NULL    | NULL |   10 |                                 |
|    2 | DERIVED     | test       | ALL  | NULL          | NULL | NULL    | NULL |   10 | Using temporary; Using filesort |
+------+-------------+------------+------+---------------+------+---------+------+------+---------------------------------+

To determine the number of distinct groups a distinct query over the grouping columns is sufficient. No temp table is needed.

SELECT COUNT(DISTINCT col1, col2) FROM tbl

Depending on the data type of your grouping columns it might be beneficial to define a two-column index on (col1, col2).

Have a look at MySQL's GROUP BY Optimization documentation to see how this works.


Flattening a 2D array to a 1D array based on indexes I am certain there is a better way to flatten the array with an optimized approach for the solution.

Example:

  [[1,2,24],[12,1241,122],[3,2],[7]] => 1 12 3 7 2 1241 2 24 122

public class flattenArray {

public static void main(String args[]) {

    int inputLists[][] = {{1,2,24},{12,1241,122},{3,2},{7}};

    flattenList(inputLists);

}

public static void flattenList(int input[][]){
    List result = new ArrayList();
    Map<Integer, List<Integer>> elements = new HashMap<Integer,List<Integer>>();
    for(int i=0;i<input.length;i++){
        for(int j=0;j<input[i].length;j++){

            List<Integer> temp = new ArrayList<Integer>();

            if(elements.get(j)!=null) {
                temp=elements.get(j);
                temp.add(input[i][j]);
                elements.put(j, temp);
            }

            else {
                temp.add(input[i][j]);
                elements.put(j,temp);
            }
        }
    }

    for(int i=0;i<input.length;i++){
        if(elements.get(i)!=null)
        result.addAll(elements.get(i));
    }

    for(int i=0;i<result.size();i++){
        System.out.println(result.get(i)+" ");
    }
}
}

Don't use raw types:

List result = new ArrayList();

Should be:

List<Integer> result = new ArrayList<>();

If you are using Java8, you could simplify the code of the map creation using the function computeIfAbsent:

for (int i = 0; i < input.length; i++){
    for (int j = 0; j < input[i].length; j++){
        elements.computeIfAbsent(j, k -> new ArrayList<>()).add(input[i][j]);
    }
}

In the next loop, you are using the break condition incorrectly.

for(int i=0;i<input.length;i++){ // input.length is not what you need
    if(elements.get(i)!=null)
        result.addAll(elements.get(i));
}

input.length is the number of arrays in your 2d array. If the length of any array is greater than the number of arrays, the code is skipping values.

This is a failed test:

Input: {{1,2,24},{12,1241,122}}
Output: 1 12 2 1241
Expected: 1 12 2 1241 24 122

You don't need to use input in the loop, you have a map with all the elements for each index, so you can make the conversion using only the map:

// Use a TreeMap instead of a HashMap to have the elements ordered by index
Map<Integer, List<Integer>> elements = new TreeMap<Integer,List<Integer>>();
...
for (List<Integer> e : elements.values()) {
    result.addAll(e);
}


Multiple files data processing in Clojure I have made a small script which grabs data into a map from 80 CSV files and calculates some statistics like average, standard deviation etc. It's also adding some additional data to map from filename.

Can you please check if it is a correct way to do so? The idea is to get a list of files in the folder using file-seq, map this to function which reads file lazy, while skipping the first line, convert all text to decimals using read-string, multiply all the data by 1e9 and then calculates all the necessary statistics.

The script works perfectly. I am just concerned about the right style to write programs in Clojure since I am pretty new to it.

Data in CSV files is just numbers in scientific format like "1.721e-9" written in one column.

(def data
  (map (fn [fsc]
     (->> (io/reader fsc)
          (line-seq)
          (rest)
          (map read-string)
          (map #(* % 1e9))
          ((fn [se]
             (let [x se
                   x2 (map #(* % %) se)

                   n (count se)
                   sum-x (reduce + x)
                   sum-x2 (reduce + x2)
                   average (/ sum-x n)
                   variance (-
                              (/ sum-x2 n)
                              (math/expt average 2))]
               (merge {:n n
                       :average average
                       :variance variance
                       :st-dev (math/sqrt variance)
                       :st-dev-sample (math/sqrt
                                        (/ (* n variance)
                                           (- n 1)))}
                      (-> (.getName fsc)
                          (clojure.string/split #"\s")
                          ((partial zipmap [:type :color :voltage :temperature]))
                          (#(assoc % :voltage (read-string (re-find #"[+-]?\d+" (% :voltage)))))
                          (#(assoc % :temperature (read-string (% :temperature))))
                          )))))))

 (->> (clojure.java.io/file "data")
      (file-seq)
      (rest))))

Don't forget about for

Consider using for instead of map in places where you use an anonymous function with map.  This particularly applies to the outer-most map, since with map what you are maping over is sort of hanging by itself at the end of the code.

(def data
  (for [fsc (->> (clojure.java.io/file "data")
                 (file-seq)
                 (rest))))]
    (->> ...)))

In this case in particular, what you are processing is made more clear with for.

Break the code up into (named) functions

In a similar vein to using for, consider breaking the code up into named functions.  While anonymous functions are obviously sufficient, a good name can bring a lot of clarity to your code.

At minimum, I would put functions like #(* % %) inside a let or letfn:

(let [square (fn [x]
               (* x x)]
  ...)

But if you think you have a use for the same function in other places, make it a top-level definition:

(defn square [x]
  (* x x))

Consider using fn in favor of #(...)

Personally, I eschew the use of #(...) reader syntax to define functions and just use fn instead.  I think the #() syntax was a well-intentioned solution to a largely non-existant problem (verbose anonymous functions), and to the extent anonymous functions are verbose in Lisps, the #() syntax is a poor replacement.  It's barely shorter than an equivalent fn form, arguably more difficult to grok (based on the questions I've seen surrounding it on stackoverflow), and has some fundamental limitations (e.g., can't be nested).

Obviously, this is a very subjective view, and certainly not everyone agrees with it.

Use comments to add clarity

Be sure to use comments to provide high-level descriptions of lower-level operations.  For example, there are a couple places where you use rest to skip the first element in a sequence.  A comment in these places indicating why this element needs to be skipped would go along way.

Likewise, consider adding comments for the major blocks of the code -- i.e., high-level descriptions of each of the loops.  Simple one-liners should be sufficient for most of these.  You might want a little more description for the mergeing part, though, since that seems to be the core of the logic.


Multi-format word counter I wrote a simple word-counter in Python a while ago, capable of processing DOCX, XLSX, TXT, and some PDF files. Now I gave it a simple GUI - this is my first tkinter program ever. What do you think?
import openpyxl
import docx2txt
import PyPDF2
import tkinter
from tkinter import filedialog

def excel_counter(filename):
    count = 0
    wb = openpyxl.load_workbook(filename)
    for sheet in wb:
        for row in sheet:
            for cell in row:
                text = str(cell.value)
                if text != "None":
                    word_list = text.split()
                    count += len(word_list)
    return count

def pdf_counter(filename):
    pdf_word_count = 0
    pdfFileObj = open(filename, "rb")
    pdfReader = PyPDF2.PdfFileReader(pdfFileObj)
    number_of_pages = pdfReader.getNumPages() - 1
    for page in range(0, number_of_pages + 1):
        page_contents = pdfReader.getPage(page - 1)
        raw_text = page_contents.extractText()
        text = raw_text.encode('utf-8')
        page_word_count = len(text.split())
        pdf_word_count += page_word_count
    return pdf_word_count

def uploadaction(event=None):
    root = tkinter.Tk()
    root.destroy()
    filename = filedialog.askopenfilename()
    return filename

def wordcounter(file):
    extension = file.split(".")[-1]
    if extension == "xlsx":
        current_count = excel_counter(file)
        return current_count
    if extension == "docx":
        text = docx2txt.process(file)
        current_count = len(text.split())
        return current_count
    if extension == "txt":
        f = open(file, "r")
        text = f.read()
        current_count = len(text.split())
        return current_count
    if extension == "pdf":
        pdf_word_count = pdf_counter(file)
        return pdf_word_count

window = tkinter.Tk()
wordcount = wordcounter(uploadaction())
label = tkinter.Label(text=f"Word Count: {wordcount}")
label.pack()
window.mainloop()

One suggestion is to simplify your top-level function by taking
advantage of a library and using a data structure to eliminate conditional logic.
import pathlib

def wordcounter(file_path):
    # Let a built-in library solve this problem.
    extension = pathlib.Path(file_path).suffix

    # Hold the counter functions in a data structure.
    # You'll need to create a couple of these.
    counters = {
        'xlsx': excel_counter,
        'docx': word_counter,
        'txt': txt_counter,
        'pdf': pdf_counter,
    }

    # Then just do what you want directly rather than checking conditions.
    return counters[extension](file_path)

Another observation is that your counter functions are duplicating the word
counting logic. Such duplication is often a signal that there are refactoring
possibilities: more specifically, the thing being duplicated might belong in
its own function. If we do that, you'll need to convert your current counter
functions (which take a path and return an integer) into reader functions (take a path
and yield chunks of text). Such a refactor is illustrated for excel_reader.
And those changes will imply a few simple adjustments in wordcounter.
def count_words(reader, file_path):
    n = 0
    for text in reader(file_path):
        n += len(text.split())
    return n

def excel_reader(file_path):
    wb = openpyxl.load_workbook(filename)
    for sheet in wb:
        for row in sheet:
            for cell in row:
                # Your current logic.
                text = str(cell.value)
                if text != "None":
                    yield text

                # I've never used openpxl, but a quick internet
                # search suggests that this is more robust conditional
                # check to filter out empty cells.
                if cell.value is not None:
                    yield str(cell.value)