"Compliment giver" mini-program I'm fairly new to programming and I decided to make a small C# console program for practice. The program asks for user input and gives a random "compliment", it also has a small chance to give a bonus "compliment". How can I improve my code and also what whould you recomend me to try adding? -ps. the "compliments" are mostly a joke.

My code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Compliment_giver
{
class Program
{
    static void Main(string[] args)
    {
        Greeting();
        start:
        string answer = Console.ReadLine();
        if(answer != "yes" && answer != "no")
        {
            Console.WriteLine("Unexpected input, please type either \"yes\" or \"no\"");
            goto start;
        }
        while(answer == "yes")
        {
            Console.Clear();
            string result = RandomCompliment();
            Console.WriteLine("\n{0}\n",result);
            if (BonusCompliment())
                Console.WriteLine("BONUS COMPLIMENT!\nYou are Very Lucky\n");
            Console.Write("Would you like another compliment? : ");
            goto start;
        }
    }

    public static string RandomCompliment()
    {
        string[] compliment = new string[10];
        compliment[0] = "Your hair smells nice";
        compliment[1] = "Your face is ok I guess";
        compliment[2] = "You're probably smarter than a penguin";
        compliment[3] = "Your body appears to be human";
        compliment[4] = "You seem pleasant";
        compliment[5] = "You likely aren't the most annoying person in the world";
        compliment[6] = "You are talanted... at something possibly";
        compliment[7] = "Your eyes are of good color";
        compliment[8] = "Your are the best looking person in a room by yourself";
        compliment[9] = "You are right all the time, exept only when you are not";

        Random number = new Random();
        int rng = number.Next(0, 10);

        string _result = compliment[rng];

        return _result;
    }

    public static void Greeting()
    {
        Console.WriteLine("Welcome to the Compliment Giver 3000!");
        Console.WriteLine("This program is designed to give free compliments to make you feel better");
        Console.Write("Do you want a compliment?    yes/no: ");
    }

    public static bool BonusCompliment()
    {
        Random percentage = new Random();
       int bonus = percentage.Next(1, 101);

        if (bonus <= 3)
            return true;
        else
            return false;
    }

}
}

Don't be discouraged by how much I have written. I'm a professional software developer and my colleagues make fun of me because of how pedantic my code reviews are. I think what you've done is actually pretty neat for a first program, and I like that you've obviously made a conscious effort to break things into useful functions (a lot of beginners don't do this well).

I always think people shouldn't pull punches early on in code reviews because it saves agro later on. Some of my points (particularly the formatting/style/naming points) will feel like pedantry, but the issue is that tiny projects become small projects become moderately sized projects and before you know it you have got a beast on your hands. So its worth getting the little stuff right early.

Formatting

Typically the class should be indented inside the namespace. having two braces at the same indentation level as you have done at the end of the snippet looks strange.
the second line of bool BonusCompliment() is not indented correctly

Naming

Choose a naming convention and stick to it religiously. This will make your code easier to read.

compliment is a sequence of compliments so should be plural
Don't abbreviate names unless its really obvious (int rng) as it makes your code harder to read. I had to guess that this stands for "random number generator", but even unabbreviated its not named correctly - it is the value returned by the generator, not the generator itself. Personally I'd probably have called it index
Why the underscore in string _result? its a common convention in some codebases to prefix class variables with an underscore as a way of distinguishing them, but this is a local variable.
C# code doesn't typically use underscores to separate words in identifier names, so your namespace looked a little out of place to me

Style

Style points are usually controversial so others might not agree with the observations I've made. Look into the point, research the opinions that are out there and form your own opinion. Once you have done this make sure you are always consistent.

You don't brace single-line if statements. Many programmers (including me) consider this to be poor practice - have a look at this
Number literals within code blocks are known as magic numbers which is a practise which is generally discouraged because they make the code harder to read. For example with (bonus <= 3) it is not really clear to the casual reader what the significance of the number '3' is. Is it the age of mum's recently purchased second hand car? The temperature that day? Give a magic number meaning by defining it as a class constant outside the method. This also means that if this number changes at a later date, the change only needs to be made in one place, reducing the possibility of errors.

public class Program {
    private const int MumsBirthday = 3;

    public bool IsItMumsBirthday(int dayOfMonth) {
        return dayOfMonth == MumsBirthday;
    }
}

Simplifications

public static string RandomCompliment() can just return compliment[rng]; rather than saving it into a variable and returning that
BonusCompliment() doesn't need the if/else. Just use return bonus <= 3

Misc

Using labels and the goto keyword is generally frowned upon in most programming languages that implement it - you can google for a fair bit of discussion on this point. As with all these kind of 'rules of thumb', if you think you have found a genuine use case that can't be better represented with other language constructs, by all means use goto. Typically though, the behaviour you are trying to implement is done with do/while like this

bool wantsMoreCompliments = true;
while(wantsMoreCompliments) {
    string answer = null;
    do {
        if(answer != null) {
            // Write error message to console
        }
        answer = Console.ReadLine()
    } while(answer != "yes" && answer != "no")
    // Do something with answer
    // Decide if wantsMoreCompliments
}

Its also worth noting that the use of goto here effectively turns your while loop into an if block - there is never going to be an instance where it loops more than once, since it gets to the end and simply jumps out of the loop.
in public static string RandomCompliment() you create an array of compliment strings. In most situations you will handle as a beginner, sequences of objects should be represented by a List<T> data structure rather than an array. For some more detailed discussion on this topic see here.
Consider using a collection initialiser for the compliments list as this is much less verbose than the way you have done it:

List<string> compliments = new List<string> {
    "Your hair smells nice",
    "Your face is ok I guess"
    // More compliments
};

Edit

In answer to your question about how to use the list with the random number generator, you would do it like this:

assuming compliments is the list we defined above:

int index = number.Next(compliments.Count);
return compliments[index];


removeClass and addClass optimization in a scoring system I have scoring system where I want to change icon color by changing CSS classes.

How can I optimize this jQuery? After testing, I found this the only alternative, but I trust there is a more simplified method.

// Count scores
$('#bodyScore').text((90 / myarray.length) * bodyScore + '/90');

// Change icon color
var bodyPercent  = bodyScore / myarray.length;
var successRatio = 0.9;
var warningRatio = 0.5;

if (bodyPercent >= successRatio) {
    $(".bkf").removeClass("label-success");
    $(".bkf").removeClass("label-warning");
    $(".bkf").removeClass("label-important");
    $(".bkf").addClass("label-success");
}

else if (bodyPercent >= warningRatio) {
    $(".bkf").removeClass("label-success");
    $(".bkf").removeClass("label-warning");
    $(".bkf").removeClass("label-important");
    $(".bkf").addClass("label-warning");
}

else {
    $(".bkf").removeClass("label-success");
    $(".bkf").removeClass("label-warning");
    $(".bkf").removeClass("label-important");
    $(".bkf").addClass("label-important");
}

Both the removeClass and addClass methods will accept a space-separated list of class names to add/remove, and both can be chained.

You can cache the selector so you don't have to repeatedly search the DOM.

And since you remove the same three classes in each branch of execution, you can move that outside of the if/else if/else:

var bkf = $(".bkf").removeClass("label-success label-warning label-important");
if (bodyPercent >= successRatio) {
    bkf.addClass("label-success");
}
else if (bodyPercent >= warningRatio) {
    bkf.addClass("label-warning");
}
else {
    bkf.addClass("label-important");
}


Simple user registry code in C++ I'm learning C++, and would like more experienced folks to review my code, and point out what could be improved/changed.
Here's the problem that should be solved (taken from Kattis Securedoors)

You need to write a simple software auditor for tracking employees
entering and exiting your building using their access cards (each card
uniquely identifies the employee holding the card). Your software
needs to look at the access log and determine if there are any
anomalies. There are two types of anomalies your software should
detect:

a user entering the building when, according to the log, they are
already supposed to be in the building
a user exiting the building
when, according to the log, they are not supposed to be in the
building

When your software sees someone enter a building (even if
it’s an anomaly), that person is assumed to be inside the building
from that point on, until he exits. Similarly, if your software sees
someone exit the building (even if it’s an anomaly), that person is
assumed to not be in the building from that point on, until he enters
again. At the beginning of the log, everyone is assumed to be outside
the building.
Input
Input Input starts with a number 1≤n≤10001≤n≤1000 indicating the
length of the log. This is followed by nn lines, each line describing
either an entry or exit by an employee. Each description is of the
form ‘entry name’ or ‘exit name’, where name is a string of up to 20
uppercase and/or lowercase characters (a-z).
Output
For each person’s entry or exit, print the name of the person,
followed by entered or exited. If the action is anomalous, print
(ANOMALY) afterward.
Sample Input 1
8
entry Abbey
entry Abbey
exit Abbey
entry Tyrone
exit Mason
entry Demetra
exit Latonya
entry Idella

Sample Output 1
Abbey entered
Abbey entered (ANOMALY)
Abbey exited
Tyrone entered
Mason exited (ANOMALY)
Demetra entered
Latonya exited (ANOMALY)
Idella entered

Here's my code (working sample)
#include <iostream>
#include <unordered_map>
#include <string>

class UserRegistry {
public:
  void UserEntered(const std::string& user) {
    RegisterUserAction(user, "entered");
  }

  void UserExited(const std::string& user) {
    RegisterUserAction(user, "exited");
  }

private:
  void LogAction(const std::string& user, const std::string& action, bool is_anomaly = false) {
    std::cout << user << " " << action << (is_anomaly ? " (ANOMALY)" : "") << std::endl;
  }

  void RegisterUserAction(const std::string& user, const std::string& action) {
    bool anomaly = false;
    if (action == "entered" && registry[user] % 2 == 1) {
      anomaly = true;
    }

    if (action == "exited" && registry[user] % 2 == 0) {
      anomaly = true;
    }

    if (!anomaly)
      registry[user]++;

    LogAction(user, action, anomaly);
  }

private:
  std::unordered_map<std::string, int> registry;
};

int main() {
  UserRegistry registry;
  int log_length;
  std::cin >> log_length;
  for (int i = 0; i < log_length; ++i) {
    std::string user, action;
    std::cin >> action >> user;

    if (action == "entry")
      registry.UserEntered(user);
    else if (action == "exit")
      registry.UserExited(user);
  }

  return 0;
}

std::unordered_map<std::string, int> is an overkill. Since you only need to track of a presence - which is naturally boolean - std::set<std::string> is enough. As a perk benefit, notice that std::set::insert indicates whether the insertion was successful or not (this is an anomaly), and std::set::erase return a number of elements erased (0 means the key was not there, another anomaly):

    std::set<std::string> registry;
    bool UserEntered(const std::string& user) {
        return !registry.insert().second;
    }
    bool userExited(const std::string& user) {
        return registry.erase(user) == 0;
    }

Now both methods return the anomaly:

    bool anomaly;
    if (action == "entered") {
        anomaly = userEntered(user);
    } else if(action == "exited") {
        anomaly = userExited(user);
    } else {
        // handle error here
        return;
    }
    LogAction(....);


Rock, paper, scissors game in Python 3 as a beginner in Python I made a rock, paper, scissors game and was wondering if my code was a bit weird and could be simplified???

print("Rock, Paper, Scissors")

def rps():
userInput = input("Please enter your choice: ")
userInput = userInput.lower()
if userInput != 'rock' and userInput != 'paper' and userInput != 'scissors':
    print("Your choice is invalid")
import random
computerChoice = ''
for x in range(1):
    num = random.randint(1,4)
if num == 1:
    computerChoice = 'rock'
elif num == 2:
    computerChoice = 'paper'
elif num == 4:
    computerChoice = 'scissors'
if userInput == computerChoice:
    print("DRAW!")
elif userInput == 'rock' and computerChoice == 'paper':
    print("COMPUTER WINS!")
elif userInput == 'rock' and computerChoice == 'scissors':
    print('USER WINS!')
elif userInput == 'paper' and computerChoice == 'rock':
    print("USER WINS!")
elif userInput == 'paper' and computerChoice == 'scissors':
    print("COMPUTER WINS!")
elif userInput == 'scissors' and computerChoice == 'rock':
    print("COMPUTER WINS!")
elif userInput == 'scissors' and computerChoice == 'paper':
    print("USER WINS!")
print("Would you like to try again?")
try_again = input("Y/N  ")
try_again = try_again.lower()
if try_again == "y":
    print("\n")
    rps()
else:
    print("\n")
    print("Thank you for using my Rock, Paper, Scissors Program")

rps()

First I suggest you to split your logic into several function, it will help you to see what is essential and what you could optimize / remove.

import random
print("Rock, Paper, Scissors")

AVAILABLE_CHOICES = ['rock', 'paper', 'scissors']

def rps():
  try:
    userInput = getUserInput()
    computerChoice = getComputerChoise()
    result = getResult()
    tryAgainMaybe()
  except Exception as err:
    print(err)

  def getUserInput() :
    userInput = input("Please enter your choice: ")
    userInput = userInput.lower()
    if userInput not in AVAILABLE_CHOICES:
        raise Exception('Your choice is invalid')
    return userInput;

  def getComputerChoice():
    return random.choice(AVAILABLE_CHOICES);

  def getResult():
    result = ''

    if userInput == computerChoice:
      result = "DRAW!"
    elif userInput == 'rock'     and computerChoice == 'scissors' or \
         userInput == 'paper'    and computerChoice == 'rock' or \
         userInput == 'scissors' and computerChoice == 'paper':
        result = 'USER WINS!'
    else:
        result = "COMPUTER WINS!"
    return result

  def tryAgainMaybe():
    print("Would you like to try again?")
    try_again = input("Y/N  ")
    try_again = try_again.lower()
    if try_again == "y":
        print("\n")
        rps()
    else:
        print("\n")
        print("Thank you for using my Rock, Paper, Scissors Program")

Create a list of available choices instead of writing them everywhere. it will be easier to modify and to test for your input values.
random allow you to select ramdomly a value in a list.
you don't need to calculate every single possibility. You can either have a draw, a win or a loose. if you don't win or draw, you loose, no computation needed here.
when you name something, try to use the same name for the same thing everything, otherwise it could be missleading. eg (userInput / computerChoice) why do we have input and choice two different words for the same kind of value ? i would choose userChoice and computerChoice instead.
Your coding convention does not respect pep8 standard. You should be using snake_case.


Are redundancy and reflection my only options here? I have a function - TriggerNotifications - that accepts a NotificationType.  The Type determines which settings I should pull from the User.

The code looks like this:

public void TriggerNotification(DbContext db, User user, NotificationType type, string content) {
            switch (type) {
                case NotificationType.Followed:
                    CreateNotification(db, user, user.Followed, content);
                    return;
                case NotificationType.Unfollowed:
                    CreateNotification(db, user, user.Unfollowed, content);
                    return;
                case NotificationType.Messaged:
                    CreateNotification(db, user, user.Messaged, content);
                    return;
                default:
                    throw new InvalidDataContractException("Invalid Notification Type");
            }
        }

So the only purpose of this method is to determine which settings should be used.  Every User has a property matching each of the enum NotificationTypes.

I think this is a good use case for reflection, but want to ask if there's any other way to reduce redundancy here.  I don't like reflection because it's not as straightforward to debug, and it's slow (right?).

UPDATE

CreateNotification:

private void CreateNotification(DbContext db, User user, NotificationSetting setting, string content) {
        // send email, notification, or both
        if (setting.Equals(NotificationSetting.None))
            return;
        var notification = new Notification() {
            UserId = user.userId
            Description = content,
            Email = email,
        };

        if (setting.Equals(NotificationSetting.Email) || setting.Equals(NotificationSetting.Both)) {
            // tell worker role to pick it up
            notification.PendingEmail = true;
        }

        // signalR stuff here

        db.Add(notification);
    }

User:

public class User : Settings {
      // other stuff
}

Settings:

public abstract class Settings {
    // notifications
    public NotificationSetting Followed { get; set; }
    public NotificationSetting Unfollowed { get; set; }
    public NotificationSetting Messaged { get; set; }
}

NotificationSetting:

// values stored in the db for each setting property - messaged, unfollowed, etc
public enum NotificationSetting {
    Navbar,
    Email,
    Both,
    None
}

NotificationType:

// values used in logic only to tell the notification builder which setting to pull
public enum NotificationType {
    Followed,
    Unfollowed,
    Messaged
}

Some of the redundancy is accidental, and some essential to your design (not essential to problem though, if it were essential to problem reflection wouldn't help).

Accidental redundancy can be removed thus:

private static NotificationSetting GetNotificationSetting(User user, NotificationType type)
{
    switch (type) {
    case NotificationType.Followed: return user.Followed;
    case NotificationType.Unfollowed: return user.Unfollowed;
    case NotificationType.Messaged: return user.Messaged;
    default:
        throw new InvalidEnumArgumentException("Invalid Notification Type");
    }
}

public void TriggerNotification(object db, User user, NotificationType type, string content) {
    CreateNotification(db, user, GetNotificationSetting(user,type), content);
}

Note:

I changed the exception as InvalidDataContractException seems to be serialization related.
GetNotificationSetting should be moved to User (or made an extension method of that class).

Now that this is out of the way,

What can you change in the design to remove redundancy?

Your user entity is not normalized. If you really were doing code-first design, User would look like this instead:

class User: Settings
{
}

public abstract class Settings {
    public IDictionary<NotificationType, NotificationSetting> NotificationSettings { get; set; }
}

Then TriggerNotification becomes just:

public void TriggerNotification(object db, User user, NotificationType type, string content) {
    CreateNotification(db, user, user.NotificationSettings[type], content);
}

Another redundancy is in NotificationSetting. It clearly is a bitmask.

[Flags]
public enum NotificationSetting : uint
{
  Navbar = 1,
  Email = 2,
// can add more like so
  Telegram = 4,
  PersonalVisit = 8,
}

Then

if (setting.Equals(NotificationSetting.Email) || setting.Equals(NotificationSetting.Both))

becomes

if (setting.HasFlag(NotificationSetting.Email))


Update table with greatest value from itself and another table I have two tables A and B with some data and a date (e.g. date of last update). The id is only unique in table A. I want to update table A with the last date (=maximum) for each id.

Table A:

id date maximum
---------------
1  5    NULL
2  6    NULL

Table B:

id date
-------
1  3
1  7
2  20

Result A after Update:

id date maximum
---------------
1  5    7
2  6    20

What I got so far works, but it looks kind of weird:

UPDATE a,b
SET a.maximum = (
  SELECT GREATEST(MAX(b.date),a.date) WHERE a.id=b.id GROUP BY b.id)
WHERE a.id = b.id
;

The double WHERE a.id=b.id confuses me especially. Can this be improved somehow?

I would update with a join to a derived table. This would likely perform better than your subselect as well as be more intuitive to read (in my opinion, at least).

UPDATE a
INNER JOIN (
    SELECT
        id,
        MAX(`date`) AS max_date
    FROM b
    GROUP BY id
) AS max_dates
    ON a.id = max_dates.id
SET a.maximum = GREATEST(a.`date`, max_dates.max_date)


Basic standalone horizontal menu Here is what I'm starting with. It is just some
 tags in a
 tag.

Arcmarks &copy;

About

Privacy

Contact

Developers

This post seems a bit unrelated as I just want to know the HTML/CSS structure for a horizontal menu.

Here is what it looks like and what I want it to look like:

Based on personal experience as well as this blog post, I'd recommend that you use an unordered list ul versus div and p, because it's a more accurate representation of the actual content you're marking up (semantic markup).

Here's a modified (div replaced with nav) snippet of the HTML from the article I linked to above:

Home

Tutorials

About

Newsletter

Contact

For the CSS to create a horizontal list, you could either use display: inline-block; or float: left; on the li items. If you need to support older browsers, like IE7, then you'll most likely want to use float. For more information on how to decide which technique to use, I'd take a look at this Stack Overflow question.


Utilize loop or other efficient way to find string and copy offset columns to certain cells I am trying to find certain strings in MasterSheet and copy offset columns (3 columns) values in the specific columns in receiver sheet. So far i have this code doing exactly what i want to do

Sub FindString()

Dim MasterSheet As Workbook     'Master sheet where data is coming from
Dim WSLHD As Worksheet          'Slave sheet receiving the data from mastersheet
Dim Findstring1 As Range        'Transfer <= 30 minutes
Dim Findstring2 As Range        'End of Month Performance (EMP)

Set MasterSheet = Workbooks.Open("\\mypath\test.xlsx")
Set WSLHD = ThisWorkbook.Worksheets("WSLHD")

'list of strings which will be searched for (this are just examples, there are many more strings)

Set Findstring1 = MasterSheet.Sheets("sheet1").Cells.Find(what:="Transfer <= 30 minutes")
Set Findstring2 = MasterSheet.Sheets("sheet1").Cells.Find(what:="End of Month Performance (EMP)")

'copy values from offset columns
WSLHD.Range("B8").Value = Findstring1.Offset(0, 1).Value
WSLHD.Range("C8").Value = Findstring1.Offset(0, 2).Value
WSLHD.Range("F8").Value = Findstring1.Offset(0, 3).Value

WSLHD.Range("B9").Value = Findstring2.Offset(0, 1).Value
WSLHD.Range("C9").Value = Findstring2.Offset(0, 2).Value
WSLHD.Range("F9").Value = Findstring2.Offset(0, 3).Value

MasterSheet.Close

end Sub

.. but i want to do this in more efficient way..i did try to use filter but that will be again filtering and choosing texts one by one so wont be any difference in terms of efficiency. Any help will be appreciated.

Option Explicit is missing from your code. From the menu at the top under Tools>Options>Editor tab>Code Settings group>Require Variable Declaration <-- toggle that check box on. It will thereafter add the Option Explicit statement to all modules. You'll have to add this manually to any currently existing modules. This mandates you have Dim foo as Range to declare your variables before usage. Not having this leads to avoidable errors. Turn it on and leave it on. Future-you will thank you for doing so.

Your Sub statement is implicitly Public since it is missing the access modifier. Writing Public Sub FindString() makes in known that you intentionally made it publicly viewable.

Indentation. Indenting your code by a tab helps identify where a Sub begins and ends. Same goes for blocks of code such as If...End IF.

Variable names are typically Camel Case. This means the first word is lower cased and the first letter of subsequent words is upper cased. If you had a variable worksheetContainingWordsToFind that would be an example of came Camel Case. Your Sub name FindString is in Pascal Case where the first letter of every word is upper cased. Subs/Function are Pascal Case while variables are Camel Case.

Your variable MasterSheet is misleading. It's actually a Workbook. Have your variables be what they say they are. Dim MasterSheet As Worksheet is an improvement to match up with what it says it is. Even better for it to be sourceSheet. That way you can have

Dim sourceBook as Workbook
Set sourceBook as Workbooks.Open("\\mypath\text.xlsx")

Dim sourceWorksheet as Worksheet
set sourceWorksheet = sourceBook.Worksheets("sheet1")

Dim foundTransferString as Range
Set foundTransferString = sourceWorksheet.Cells.Find(What:="Transfer <= 30 minutes")

Dim foundEMPString as Range
Set foundEMPString = sourceWorksheet.Cells.Find(What:="End of Month Performance (EMP)"

which has your code self documenting.

The above code also eliminates a wall of declarations at the start. I'm of the opinion that declaring a variable just before you use it helps aid in refactoring and eliminating unused variables. Rather than needing to right click on the variable to display the context menu>Definition to find where it's declared you have it right above the first use.

The code ThisWorkbook.Worksheets("WSLHD") can use the object (Excel Worksheet) directly. In the Project Explorer, found in the menu at the top under View>Project Explorer (Hotkey: Ctrl+R). Double click the sheet under Microsoft Excel Object to display the code behind pane for that Worksheet object. Under View>Properties Window (Hotkey: F4) display the properties window. At the very top where it says (Name) you can rename the worksheet CodeName to a more descriptive name. I kept WSLHD as I don't know what that means, another reason to have self describing names. Thereafter you can use that object directly with WSLHD.Cells.Find instead of in the roundabout way of assigning a worksheet variable with ThisWorkbook.Worksheets("WSLHD"). The latter is prone to breaking if the tab is renamed.

Your comment "... there are many more strings" is a sign-post indicating you want a function. This function has 2 input variables that you provide as arguments. It searches the sheet you indicate looking for the string value. If it finds the result it sets the last variable outFoundCell which is passed ByRef so the assignment applies is reflected in the calling procedure. If the string was found the TryFind = Not outFoundCell Is Nothing assigns True to the function, otherwise it assigns False.

Private Function TryFind(ByVal searchForValue As String, ByVal sourceSheet As Worksheet, ByRef outFoundCell As Range) As Boolean
    Set outFoundCell = sourceSheet.Cells.Find(What:=searchForValue)
    TryFind = Not outFoundCell Is Nothing
End Function

You couple that with a method to populate the cells

Private Sub PopulateCellsWithOffsetResults(ByVal foundCell As Range, ByVal populationSheet As Worksheet, ByVal populationRow As Long)
    populationSheet.Cells(populationRow, "B").Value2 = foundCell.Offset(0, 1).Value2
    populationSheet.Cells(populationRow, "C").Value2 = foundCell.Offset(0, 2).Value2
    populationSheet.Cells(populationRow, "F").Value2 = foundCell.Offset(0, 3).Value2
End Sub

And you can then invoke the function in the following manner. You first search for the string. If it's found it then enters the true part of the If...Then...Else Statement and populates the information.

If TryFind("Transfer <= 30 minutes", sourceWorksheet, foundCell) Then
    PopulateCellsWithOffsetResults foundCell, WSLHD, 8
End If

Doing all that you end up with the code below. There is more that can be done because the population rows 8, and 9 are static numbers. You probably want a dynamic row that changes as things are populated.

Option Explicit

Public Sub SearchForStringsAndPopulatedOffsetValuesOnAnotherWorksheet()
    Dim sourceBook As Workbook
    Set sourceBook = Workbooks.Open("\\mypath\test.xlsx") 'Master sheet where data is coming from
    Dim sourceWorksheet As Worksheet
    Set sourceWorksheet = sourceBook.Sheets("sheet1")

    Dim foundTransferCell As Range
    If TryFind("Transfer <= 30 minutes", sourceWorksheet, foundTransferCell) Then
        PopulateCellsWithOffsetResults foundTransferCell, WSLHD, 8
    End If

    Dim foundEndOfMonthPerformanceCell As Range
    If TryFind("End of Month Performance (EMP)", sourceWorksheet, foundEndOfMonthPerformanceCell) Then
        PopulateCellsWithOffsetResults foundEndOfMonthPerformanceCell, WSLHD, 9
    End If

    sourceBook.Close
End Sub

Private Sub PopulateCellsWithOffsetResults(ByVal foundCell As Range, ByVal populationSheet As Worksheet, ByVal populationRow As Long)
    populationSheet.Cells(populationRow, "B").Value2 = foundCell.Offset(0, 1).Value2
    populationSheet.Cells(populationRow, "C").Value2 = foundCell.Offset(0, 2).Value2
    populationSheet.Cells(populationRow, "F").Value2 = foundCell.Offset(0, 3).Value2
End Sub

Private Function TryFind(ByVal searchForValue As String, ByVal sourceSheet As Worksheet, ByRef outFoundCell As Range) As Boolean
    Set outFoundCell = sourceSheet.Cells.Find(What:=searchForValue)
    TryFind = Not outFoundCell Is Nothing
End Function

The Sub PopulateCellsWithOffsetResults and Function TryFind are Private and thereby not visible outside of the module because they are an implementation detail. You aren't concerned with how they work, rather that they do work as evidenced when you run SearchForStringsAndPopulatedOffsetValuesOnAnotherWorksheet which should have a better more descriptive name. That's just what I came up with since I really don't know everything that you're populating.


PE4: Largest Palindrome Product (Clojure) I solved Project Euler 4 using Clojure Lisp.

  A palindromic number reads the same both ways. The largest palindrome made from the product of two 2-digit numbers is 9009 = 91 × 99.
  Find the largest palindrome made from the product of two 3-digit numbers.

I'm not very experienced with Clojure yet. I tried to break the logic down into small, easy to understand functions. I added documentation and input type assertions to each function.

How can I improve this code? Is :pre a good method for asserting correct input types, or is there a better way of doing that?

(defn palindrome? [arg]
  "Returns true if arg is a palindrome, i.e., it reads the same both ways"
  (= (seq (str arg))
     (reverse (str arg))))

(defn pow [N X]
  "Returns N to the power of X"
  {:pre [(number? N) (number? X)]}
  (reduce * (repeat X N)))

(defn nums-with-n-digits [digits]
  "Return list of all numbers comprised of a certain number of digits"
  {:pre (number? digits)}
  (let [max (pow 10 digits)
        min (/ max 10)]
    (range min max)))

(defn products-of-nums-list [nums]
  "Returns a descending order list of distinct products of each of the numbers in nums"
  {:pre (list? nums)}
  (sort > (distinct (for [x nums y nums] (* x y)))))

(defn solve-pe4 [digits]
  "Returns list of solutions of largest palindrome products of all numbers with a certain number of digits"
  {:pre (number? digits)}
  (first (filter palindrome? (products-of-nums-list (nums-with-n-digits digits)))))

(defn -main []
  (println (solve-pe4 2)) ; 9009
  (println (solve-pe4 3))); correct answer

A function's docstring must go before its argument vector. You've written your docstrings after your argument vectors, which prevents your :preconditions from being recognized and doesn't work when you try to retrieve the docstring with tools like doc:

(doc palindrome?)
;; -------------------------
;; user/palindrome?
;; ([arg])
;;   nil

Rather than explicitly calling seq in your palindrome? function, you could use clojure.string/reverse and then just compare the two strings. Also, in Clojure code, if when you have something that could be anything, or something that could be any number, it's conventional to name it x:

(require '[clojure.string :as str])

(defn palindrome?
  "Returns true iff x reads the same both ways in base ten."
  [x]
  (let [s (str x)]
    (= s (str/reverse s))))

In most cases, it's conventional to use lowercase instead of uppercase for names. Following from the rule of x above, if you have two things, each of which could be any number, it's conventional to name the second one y:

(defn pow
  "Returns x to the power of y."
  [x y]
  (reduce * (repeat y x)))

It's essential that the collection of :preconditions you give in a defn be a collection of preconditions (such as the vector you have in your pow function), rather than just a single precondition (such as in your other three functions with preconditions). This is because, due to Clojure's homoiconicity, (number? digits) is, in fact, a collection (a list, to be precise) containing two elements: number? and digits. In this case, you can use {:pre [(number? digits)]} instead.

Your local max and min bindings shadow the existing max and min functions in clojure.core, which is generally a bad idea because it can lead to confusion and forces you to use namespace-qualified names for those functions if you want to use them. Also, since you already use the name n in your function name (nums-with-n-digits), using n as your parameter name would allow you to simplify your function's docstring and body without sacrificing clarity:

(defn nums-with-n-digits
  "Return a lazy sequence of all positive integers with n digits."
  [n]
  (let [end (pow 10 n)
        start (/ end 10)]
    (range start end)))

If you move the docstring for products-of-nums-list and replace the condition map with {:pre [(list? nums)]}, you'll see that calls like (products-of-nums-list (nums-with-n-digits 2)) will fail. This is because range (and therefore nums-with-n-digits) doesn't return a list; it returns a sequence. All lists are sequences, but not all sequences are lists. While (range Long/MAX_VALUE) returns instantly and uses negligible memory, (apply list (range Long/MAX_VALUE)) will use all your computer's memory before it returns.

Your products-of-nums-list function is pretty scary, actually. Your use of distinct and sort causes first a set and then an array containing all of the distinct elements in nums to be held in memory. Try evaluating (products-of-nums-list (nums-with-n-digits 4)) while watching your CPU and memory usage.

Rather than delegating the task of sorting (which is overkill anyway) to a products-of-nums-list function that isn't particularly general anyway, it would be better to just put the for directly into solve-pe4 and use max instead of sort to get the final result:

(defn solve-pe4
  "Returns the largest palindrome made from the product of two n-digit numbers."
  [n]
  (->> (let [nums (nums-with-n-digits n)]
         (for [x nums
               y nums]
           (* x y)))
       (filter palindrome?)
       (apply max)))

This function is already quite readable, so I don't see a need to decompose it further. It significantly improves the performance of (solve-pe4 3) and, given enough time, can find solutions to larger problems:

(solve-pe4 4)
;;=> 99000099

Your :preconditions are an OK way to assert correct input types. Clojure 1.9 will bring the spec library, which is a far more powerful way to solve that problem (and others):

(require '[clojure.spec :as s])

(s/fdef pow
  :args (s/cat :x number? :y nat-int?)
  :ret number?)

(s/fdef nums-with-n-digits
  :args (s/cat :n pos-int?)
  :ret (s/coll-of pos-int?))

(s/fdef solve-pe4
  :args (s/cat :n pos-int?)
  :ret pos-int?)

You can use instrument to enable spec validation:

(require '[clojure.spec.test :as stest])

(stest/instrument)

(nums-with-n-digits 0)
;; clojure.lang.ExceptionInfo: Call to #'user/nums-with-n-digits did not conform to spec:
;;                             In: [0] val: 0 fails at: [:args :n] predicate: pos-int?
;;                             :clojure.spec/args  (0)
;;                             :clojure.spec/failure  :instrument


JavaScript DOM modification I'm thinking about DOM modification performance.

In a perfect way, we alter each element but DOM modification is really heavy and alter many elements at same time froze the browser. I didn't make any performance tests but I want to know if the second way can be more powerful.

In my head, the first way will ask the browser to refresh the display for each div. So logically, with the second way, the browser will refresh only once per injection. Is that correct?

HTML:

If you have performance issues, then I believe you should first try and profile your script, to identify which function(s) run slowly. There are profilers embedded in most scripting consoles.

Regarding your question: since your code should not trigger any repaint, I believe that method 1 would be fine. Unfortunately, I have not been able to reproduce the issue on my machine, so cannot confirm this answer.

To improve performance, I suppose you could try the following:

Rather than using setInterval, consider chaining setTimeout calls, so that the browser remains responsive.
Try and remove your library, to use native Javascript. It looks like you do not need anything more than getElementById, getElementsByTagName, getAttribute and setAttribute - so should be able to safely eliminate any library overhead.
Possibly cache the retrieval of all the div, as you only need to perform it once.

So basically, try and run the following (untested) on window.onload:

function update() {
    var delay = 100;
    var container = document.getElementById("container");
    var divs = container.getElementsByTagName("div");

    function updateDivs() {
        for (var ii = 0; ii < divs.length; ii++) {
            var div = divs[ii];
            if (randNum(0, 10) == 1) {
                var value = randString();
                if (div.getAttribute("alt") != value) {
                    div.setAttribute("alt", value);
                }
            }
        }
        setTimeout(arguments.callee, delay);
    }

    updateDivs();
}


Delete each character in one string that matches any character in another I am trying to solve an exercise in which I am given the following function:

/* squeeze: delete all c from s */
void squeeze(char s[], int c)
{
 int i, j;

 for (i = j = 0; s[i] != '\0'; i++)
     if (s[i] != c)
       s[j++] = s[i];
 s[j] = '\0';
}

the exercise is:

  Write an alternative version of squeeze(s1,s2) that deletes each
  character in s1 that matches any character in the string s2.

I have came up with this:

#define COPY   1
#define NOCOPY 0

void multisqueeze(char s[], char f[])
{
int i, j,k, state;
i = j = k = 0;
state = COPY;
while (s[i] != '\0')
{
    state = COPY;
    while (f[k] != '\0' && state == COPY)
    {

        printf("s is %c, f is %c\n", s[i], f[k]);
        if (tolower(f[k]) == tolower(s[i]))
        {
            state = NOCOPY;
            printf("Nocopy set \n");
        }
        k++;
    }
    k=0;

    if (state == COPY)
    {
        printf("Copying a character, %c\n", s[i]);
        s[j++] = s[i];
    }
    i++;
}
printf("Loop ended, s is %c\n", s[i]);
s[j] = '\0';
}

This seems to be a valid solution for the problem and outputs the correct answer.  But I have been told that the implementation is messy and not readable because of the use of the state variable and that there are more elegant ways of doing it. I have been trying to wrap my head around it for some time and I simply can't see a way to not use the state variable at all. I think this exercise is from The C programming Language by Brian Kernighan and Dennis Ritchie.

What you have is an O(n^2) solution.

But lets look at tidying it up first:

Do you really need these macros.
If you must have them then why not name the variable copy and use the macros TRUE/FALSE

#define COPY   1
#define NOCOPY 0

Declaring variables using the comma is lazy and considered bad practice, but you should also use more descriptive variable names. i/j/k do not explain what they are being used for. Also if you function is longer and you need to change it looking for all instances of a variable 'i' may take a while longer because of all the false positives on a search so maintenance wise it is also a bad idea.
There are a couple of edge cases where the declaration can go wrong. So in general it is just best to stay clear.

int i, j,k, state;

You are using a while() loop. But a for(;;) loop would have been neater. It allows initialization test and increment in the same statement. This puts all work done on the k variable in the same place.

    while (f[k] != '\0' && state == COPY)

Are you sure you actually want to do this test?

        if (tolower(f[k]) == tolower(s[i]))

OK. The heart of the problem. Use of the state variable.
The reason you have this test is to break out of this while loop and then avoid doing the actual work in the next loop. You already have the failed state information implicitly by the place you are in the loop while testing so all you need to do is break out of the current loop. You do not need to do an explicit test in the condition but just use a break statement.

            state = NOCOPY;

So if we re-write your version taking into account my comments we get:

void multisqueeze(char s[], char f[])
{
    int i     = 0;  // Iterate over s[]
    int j     = 0;  // Iterate over s[] as elements are copied (this is the dst)

    for(i = 0; s[i] != '\0'; ++i)
    {
        int k     = 0;                    // Iterate over f[]
        for(k = 0; f[k] != '\0'; ++k)
        {
            if (tolower(f[k]) == tolower(s[i]))
            {
                break;
            }
        }

        if (f[k] != '\0') // Then we never reached the end of the loop
        {                 // This means we found a match in the f[] array
            s[j++] = s[i];
        }
    }
    s[j] = '\0';
}

But there is an O(n) solution.
What you need to do is convert the search of the second string into a single comparison.

Since char is a limited range (0-255) we can swap space for time.
Allocate an array that represents every character possible character. Then we can use this as a way to look-up the existence of a character.

So rather than doing this:

        for(k = 0; f[k] != '\0'; ++k)
        {
            if (tolower(f[k]) == tolower(s[i]))
            {
                break;
            }
        }

for each iteration of s[]. What you want to do is iterate over f[] once and save the state of each character. So that you can just look up the answer in one step rather than an iteration. To do this we hoist this loop out of the inner loop and allocate an array for all possible values of char, then mark any we find in f[] as true in our allocated array.

This is what mine would look like (actually mine would look slightly different as I would have written in the C++ style but I have translated into C to make it consistent with the question).

void squeeze(char s[], char f[])
{
    int   loopF;
    int   loopS;
    int   squeeze   = 0;
    int   test[256] = {0};            // One for each character. Default to false.
                                      // I would use bool here. But C does not have bool
                                      // Could use char to save space but that can be confusing
                                      // as not everybody considers a char as an integer type.

    for(loopF = 0; f[loopF] != '\0'; ++loopF)
    {    test[tolower(f[loopF])]  = 1;    // Mark every character in test that is in f as true.
         test[toupper(f[loopF])]  = 1;    // Thus we can test for existence just by looking it up
    }

    for(int loopS=0; s[loopS]; ++loopS)
    {
          if (test[s[loopS]])          // Test to see if the character in s is also marked true in test.
          {    squeeze++;
               continue;
          }
          // move characters down the squeezed amount
          s[loopS-squeeze] = s[loopS];
    }
}


Bounded blocking queue Can someone please review this code for me? I have not implemented all the methods for simplicity.

/**
 * Implements a blocking bounded queue from a given non-blocking unbounded queue implementation
 */
abstract class DerivedQueue<E> implements Queue<E>
{
    DerivedQueue(Queue<E> queue, int size)
    {
        if(queue==null | size <0)
        {
            throw new RuntimeException("Bad Input");
        }
        fQueue = queue;
        fSize = size;
    }

    Queue<E> fQueue;
    int fSize;
    int fCnt;

    @Override
    public boolean addAll(Collection<? extends E> arg0)
    {
        return false;
    }

    @Override
    public boolean isEmpty()
    {
        return (fCnt==0);
    }

    public E remove()
    {
        if(fCnt==0)
        {
            try
            {
                wait();
            }
            catch (InterruptedException e)
            {
                throw new RuntimeException("Waiting thread was interrupted during remove with msg:",e);
            }
        }

        E elem =  fQueue.remove();
        fCnt--;
        notifyAll();
        return elem;
    }

    @Override
    public boolean add(E elem)
    {
        if(fCnt == fSize)
        {
            try
            {
                wait();
            }
            catch (InterruptedException e)
            {
                throw new RuntimeException("Waiting thread was interrupted during remove with msg:",e);
            }
        }

        return fQueue.add(elem);
    }

}

Open questions:

Would calling notifyAll() lead to multiple waiting threads checking the while() condition at the same time, and hence there is a possibility that before the while gets satisfied, 2 threads are already out of it causing an outOfBound exception? Or does the methods need to be marked synchronized at all, and instead I can use a synchronized block for reading or writing part of the array while the checking of constrained and waiting can remain unsynchronized?
Is there a way to notify precisely only the consumer threads (waiting in take) or only the producer (waiting input) threads in this example? notifyAll notifies all of them.

This problem screams "semaphore".

Have a semaphore whose count represents the number of slots available in the queue.  Enqueuers decrement the semaphore.  Dequeuers increment it.  This effectively caps the amount of nodes in the queue to some maximum, namely the initial value of the semaphore.

Likewise you can have a different semaphore represent the number of items currently enqueued.  In this case dequeuers decrement it and enqueuers increment.  For this one, the initial value is zero.  This allows dequeuers to block until an item is available.


UITableView, auto locate back at indexPath when user drags it I have a tableView, which auto locates back at indexPath after user has dragged it.

In other words, bouncing the tableView back into its bounds, after user dragged it out.

Here is the code:

// ...

var tracking = false
var currentPath = 0

@objc func updateRecorder(_ timer: Timer){
     currentPath = somethingCalculated
}

// ...

extension ViewController: UITableViewDelegate{

    func scrollViewDidEndDragging(_ scrollView: UIScrollView, willDecelerate decelerate: Bool) {
            // when the scene is in a special state
            tracking = true
        }
    }

    func scrollViewDidEndDecelerating(_ scrollView: UIScrollView) {
        // when the scene is in the same special state
        if tracking == true{
            tracking = false
            scrollToRow(at: IndexPath(row: currentPath, section: 0), at: UITableView.ScrollPosition.middle, animated: true)
        }
    }
}

In fact, it is a music tracking app. People sing a song, the lyric tableView tracks the voice, and it auto scrolls to current line.

When tracking, and people scrolls by hand, the lyric tableView should bounces back to it ought to be.

The code above works.

Any other ideas?

A couple of observations:

You are handling scrollViewDidEndDecelerating. But what if there was no deceleration? E.g. you’d generally want to call some common routine, e.g. didFinishUserScroll, either way:

func scrollViewDidEndDragging(_ scrollView: UIScrollView, willDecelerate decelerate: Bool) {
    if !decelerate {
        didFinishUserScroll()
    }
}

func scrollViewDidEndDecelerating(_ scrollView: UIScrollView) {
    didFinishUserScroll()
}

It looks like you are scrolling as soon as deceleration finishes. I would suggest that you only want to scroll at the end of the user gesture (whether decelerating or not) only if there is an update to the position in the lyrics in the intervening period of time.

E.g. you’re bopping along in a song, the drummer goes off into some lengthy, lyric-free solo, so the bored user starts scrolling through the lyrics themselves. When they let go, if the drum solo is still in progress, I’d suggest that you might not want to scroll back until the singing resumes. I wouldn’t necessarily scroll back immediately.

Now maybe you’ve got the sort of logic in the code you didn’t share with us, and if so, don’t worry about it. But, frankly, the fact that tracking is so ambiguous is code smell. (I.e., is it a boolean whether a touch gesture is in progress? is it indicating that we’re following along with lyrics at all? is it that there are updates to progress in the song that requires update of the position?) The fact that the intent of tracking is so ambiguous suggests you might consider a better name for it.
It looks like currentPath is an Int, a row number. So I wouldn’t call it currentPath, but rather currentRow.
When you have a boolean, you generally just check the value, not compare it to another boolean. So, for example, instead of:

if tracking == true {
    tracking = false
    scrollToRow(at: IndexPath(row: currentPath, section: 0), at: UITableView.ScrollPosition.middle, animated: true)
}

You would do:

if tracking {
    tracking = false
    scrollToRow(at: IndexPath(row: currentPath, section: 0), at: .middle, animated: true)
}

I also removed the redundant UITableView.ScrollPosition reference.
This is beyond the scope of this question, but the fact that you’re calling scrollToRow(at:at:animated:) without referencing the table view worries me, making me wonder whether you have all of this business logic buried inside your table view. We generally strive to isolate our UI from business logic of the app.


Portfolio object made of Asset classes and Securities I am learning Swift and object oriented programming. I am new to design patterns and protocol oriented programming.

In my test code I have several related objects:

Security: the most important object that will be contained in the other ones.
AssetClass: Contains Security objects
Portfolio:  Contains Security and AssetClass objects
Watchlist: Contains Security objects. Think about it as a Portfolio object that cannot contain AssetClass objects, only Security objects.

I want to be the most abstract as I can to avoid tight coupling.

The following code works but I am wondering how it can be improved. Do you see major weak points in my design?

import Foundation

protocol SecurityType{
    var code : String {get set}
    var name : String? {get set}
}

class Security:SecurityType{

    var code : String = ""
    var name : String?

    init(withCode aCode: String, andName aName: String?){
        self.code = aCode
        if let securityName = aName{
            self.name = securityName
        }
    }
}

protocol SecuritiesBasketType{
    var name : String {get set}
    var securities : [SecurityType]{get set}
    init()
}

extension SecuritiesBasketType{

    //Default Initializers

    init(withName aName: String){
        self.init()
        self.name = aName
    }

    init(withName aName: String, andSecurities securitiesArray:[SecurityType]){
        self.init()
        self.name = aName

        for aSecurity : SecurityType in securitiesArray{
            securities.append(aSecurity)
        }
    }

    mutating func addSecurity(security: SecurityType){
        securities.append(security)
    }
}

class AssetClass : SecuritiesBasketType{

    var name : String = ""
    var securities = [SecurityType]()
    required init(){}
}

class WatchList: SecuritiesBasketType{

    var name : String = ""
    var securities = [SecurityType]()
    required init(){}
}

class Portfolio{

    var name : String = ""
    var securities = [Security]()
    var assetClasses = [AssetClass]()

    init(withName aName: String){
        self.name = aName
    }

    init(withName aName: String, andAssetClasses assetClasses:[AssetClass]){
        self.name = aName
        for assetClass in assetClasses{
            self.assetClasses.append(assetClass)
        }
    }

    init(withName aName: String, andAssetClasses assetClasses:[AssetClass], andSecurities securities:[Security]){

        self.name = aName

        for assetClass in assetClasses{
            self.assetClasses.append(assetClass)
        }

        for security in securities{
            self.securities.append(security)
        }
    }

}

//Creating Securities
var sMicrosoft = Security(withCode: "MSFT", andName: "Microsoft")
var sApple = Security(withCode: "AAPL", andName: "Apple")
var sJPMorgan = Security(withCode: "JPM", andName: "JP Morgan")
var sBankOfAmerica = Security(withCode: "BAC", andName: "Bank of America")
var sCaterpillar = Security(withCode: "CAT", andName: "Caterpillar")

//Creating Asset Classes
var acBanks = AssetClass(withName: "Banks", andSecurities: [sJPMorgan, sBankOfAmerica])
var acTechnology = AssetClass(withName: "Technology", andSecurities: [sMicrosoft, sApple])

//Creating Watchlists
var watchList1 = WatchList(withName: "List1", andSecurities: [sMicrosoft,sJPMorgan,sCaterpillar])
var watchList2 = WatchList(withName: "List2", andSecurities: [sBankOfAmerica,sApple])

//Creating Portfolios

var portfolio1 = Portfolio(withName: "Portfolio1", andAssetClasses: [acBanks,acTechnology])
var portfolio2 = Portfolio(withName: "Portfolio2", andAssetClasses: [acBanks, acTechnology], andSecurities: [sCaterpillar])

//Test

for ac : AssetClass in portfolio2.assetClasses{
    for s : SecurityType in ac.securities{
        print("\(s.name!)")
    }
}
for s : SecurityType in portfolio2.securities{
    print("\(s.name!)")
}

I've got a problem with your SecuritiesBasketType protocol and its extension.  The problem is best highlighted by taking a look at the multiple classes you've implemented to conform to this protocol.

class AssetClass : SecuritiesBasketType{

    var name : String = ""
    var securities = [SecurityType]()
    required init(){}
}

class WatchList: SecuritiesBasketType{

    var name : String = ""
    var securities = [SecurityType]()
    required init(){}
}

There are a couple of problems here.  First, why do we need two classes that are identical in every way except name?  We don't.

I understand that we will want assets that we actually own as well as assets that we're watching to help us make future decisions, but making different classes like this simply does not make sense.  Additionally, this name property makes relatively little sense, and having a zero-argument constructor plus var properties is problematic as far as I'm concerned.

But we must have the zero argument constructor because of the problem with the protocol.  We have conformed to the SecuritiesBasketType, which has a zero-argument initializer as part of its conformance.  This means that everything that conforms to this protocol as well as things that inherit from that must implement this zero-argument constructor with the required keyword.

In some cases, a required initializer makes sense.  Like, for example, with the NSCoding protocol (more reading).  But here, we've only required this zero argument constructor so that we can add two other constructors in the protocol extension.

But WHY?

What you have here is a protocol that should be a class.  You've done all of the work in the protocol and protocol extension, and it has resulted in very weird things.  It doesn't make much sense to subclass anything that conforms to this protocol and it doesn't make much sense to create more than one class that conforms to this protocol.  And when those two things are true (and they are here), then it doesn't make much sense to have the protocol at all.  We should instead have a class that looks something like this:

class SecuritiesBasket {
    let name: String
    var securities: [SecurityType]

    init(withName name: String, securities: [SecurityType]) {
        self.name = name
        self.securities = securities
    }

    func addSecurity(security: SecurityType) {
        securities.append(security)
    }
}

Now the SecuritiesBasket class does everything we wanted out of the SecuritiesBasketType protocol, but it does it without forcing any required initializer (which severely dictates a lot of the things you do with subclasses from that point).

In Swift, we want to adopt a protocol-oriented approach to programming.  Protocols allow us to do some powerful things.  But with that said, we need to use the right tool for the right job.

We use protocols to allow multiple implementations of the same interface.  The most classic example is with UITableView and its delegate/datasource properties.  All of the UI logic of the table view exists within UIKit, yet with just a handful of methods in the protocols, the table view can adapt to display whatever information you need it to display.


Applying a dataframe function to a pandas groupby object I am trying to apply a function to each group in a pandas dataframe where the function requires access to the entire group (as opposed to just one row). For this I am iterating over each group in the groupby object. Is this the best way to achieve this?

import pandas as pd
df = pd.DataFrame({'id': [1,1,1,1,2,2,2],
                   'value': [70,10,20,100,50,5,33],
                   'other_value': [2.3, 3.3, 7.4, 1.1, 5, 10.3, 12]})
def clean_df(df, v_col, other_col):
    '''This function is just a made up example and might
       get more complex in real life. ;)
    '''
    prev_points = df[v_col].shift(1)
    next_points = df[v_col].shift(-1)
    return df[(prev_points > 50) | (next_points < 20)]
grouped = df.groupby('id')
pd.concat([clean_df(group, 'value', 'other_value') for _, group in grouped])

The original dataframe is

    id  other_value value
0   1   2.3         70
1   1   3.3         10
2   1   7.4         20
3   1   1.1         100
4   2   5.0         50
5   2   10.3        5
6   2   12.0        33

The code will reduce it to

    id  other_value value
0   1   2.3         70
1   1   3.3         10
4   2   5.0         50

You can directly use apply on the grouped dataframe and it will be passed the whole group:

def clean_df(df, v_col='value', other_col='other_value'):
    '''This function is just a made up example and might
       get more complex in real life. ;)
    '''
    prev_points = df[v_col].shift(1)
    next_points = df[v_col].shift(-1)
    return df[(prev_points > 50) | (next_points < 20)]

df.groupby('id').apply(clean_df).reset_index(level=0, drop=True)
#    id  other_value  value
# 0   1          2.3     70
# 1   1          3.3     10
# 4   2          5.0     50

Note that I had to give the other arguments default values, since the function that is applied needs to have only one argument. Another way around this is to make a function that returns the function:

def clean_df(v_col, other_col):
    '''This function is just a made up example and might
       get more complex in real life. ;)
    '''
    def wrapper(df):
        prev_points = df[v_col].shift(1)
        next_points = df[v_col].shift(-1)
        return df[(prev_points > 50) | (next_points < 20)]
    return wrapper

Which you can use like this:

df.groupby('id').apply(clean_df('value', 'other_value')).reset_index(level=0, drop=True)

Or you can use functools.partial with your clean_df:

from functools import partial

df.groupby('id') \
  .apply(partial(clean_df, v_col='value', other_col='other_value')) \
  .reset_index(level=0, drop=True)


Reverse dictionary where values are lists I'm working on the following problem: I've got a dictionary like this one:

dic={0:[0,1,2],1:[3,4,5]}

And I want to reverse it so it looks like this:

dic2={0:0,1:0,2:0,3:1,4:1,5:1}

I managed to make it, but doing this:

dic2={}
for i in dic:
    for j in dic[i]:
        dic2[j]=i

I know about list and dict comprehensions and this code reeks of it, but I'm not good at them when there are nested for and dicts. How would you make it more efficiently?

There's really not much to say about this code, it is straightforward.

Style

These are only nitpicks.

Generic dictionary keys are typically named k instead of i or j, but in a specific application a more descriptive name would be even better.
Collections should be named by their purpose in the application, not their type.
By convention, assignments and other binary operators should be surrounded by spaces: a = b, not a=b. See PEP 8 for reference, if you're not already aware of it. Following this style guide makes your code easier to read for others.

Code improvements

When iterating over the keys and values of a dictionary at the same time, you can use

for k, v in dic.items():
    # ... use k, v ...

instead of

for k in dic:
    v = dic[k]
    # ...

The nested loop can be transformed to a dictionary comprehension like this:

dic2 = {v: k for k, values in dic.items() for v in values}

You can remember that the order of for clauses in the comprehension is the same as the order of corresponding nested for loops.

Potential pitfalls

You should be aware that this transformation from a dictionary of lists to a reverse dictionary only works if all items in the original lists are unique. Counterexample:

>>> dic = {0: [1, 2], 1: [2, 3]}
>>> {v: k for k, values in dic.items() for v in values}
{1: 0, 2: 1, 3: 1}  # missing 2: 0

To correct this, the output dictionary should have the same format as the input, namely mapping each of the items in the input lists to a list of corresponding keys.

If the input represents a directed graph (mapping nodes to lists of neighbours), this corresponds to computing the transposed or reversed graph. It can be done by using a collections.defaultdict. I don't see an easy way to write it as a comprehension in this case.

from collections import defaultdict

graph = {0: [1, 2], 1: [2, 3]}

transposed_graph = defaultdict(list)
for node, neighbours in graph.items():
    for neighbour in neighbours:
        transposed_graph[neighbour].append(node)

# {1: [0], 2: [0, 1], 3: [1]}


Thumbnails for bootstrap carousel jQuery plugin Codepen: http://codepen.io/srkimir/pen/mGbrf
Github: https://github.com/srkimir/thumbnails-carousel

While you slide left or right appropriately, thumbnails gets selected and change their opacity to be different among others thumbnails.  You can also click on thumbnails to show the appropriate image on carousel.

HTML

Add child
 of thumbnails in the end of  carousel bootstrap parent, like:

    ...

CSS

ul.thumbnails-carousel {
    padding: 5px 0 0 0;
    margin: 0;
    list-style-type: none;
    text-align: center;
}
ul.thumbnails-carousel .center {
    display: inline-block;
}
ul.thumbnails-carousel li {
    margin-right: 5px;
    float: left;
    cursor: pointer;
}
.controls-background-reset {
    background: none !important;
}
.active-thumbnail {
    opacity: 0.4;
}
.indicators-fix {
    bottom: 70px;
}

JavaScript:

// thumbnails.carousel.js jQuery plugin
;(function(window, $, undefined) {

    var conf = {
        center: true,
        backgroundControl: false
    };

    var cache = {
        $carouselContainer: $('.thumbnails-carousel').parent(),
        $thumbnailsLi: $('.thumbnails-carousel li'),
        $controls: $('.thumbnails-carousel').parent().find('.carousel-control')
    };

    function init() {
        cache.$carouselContainer.find('ol.carousel-indicators').addClass('indicators-fix');
        cache.$thumbnailsLi.first().addClass('active-thumbnail');

        if(!conf.backgroundControl) {
            cache.$carouselContainer.find('.carousel-control').addClass('controls-background-reset');
        }
        else {
            cache.$controls.height(cache.$carouselContainer.find('.carousel-inner').height());
        }

        if(conf.center) {
            cache.$thumbnailsLi.wrapAll("

");
        }
    }

    function refreshOpacities(domEl) {
        cache.$thumbnailsLi.removeClass('active-thumbnail');
        cache.$thumbnailsLi.eq($(domEl).index()).addClass('active-thumbnail');
    }

    function bindUiActions() {
        cache.$carouselContainer.on('slide.bs.carousel', function(e) {
            refreshOpacities(e.relatedTarget);
        });

        cache.$thumbnailsLi.click(function(){
            cache.$carouselContainer.carousel($(this).index());
        });
    }

    $.fn.thumbnailsCarousel = function(options) {
        conf = $.extend(conf, options);

        init();
        bindUiActions();

        return this;
    }

})(window, jQuery);

// Kick it
$('.thumbnails-carousel').thumbnailsCarousel({});

Additionally, you can pass center (default: true) and backgroundControl (default: false) if you want (or no) to center your thumbnails or backgroundControl if you want (or no) controls background which is activated on hover in bootstrap carousel.

The only thing worse than missing alt text is bad alt text. 1_tn.jpg is meaningless. Instead, use empty alt text.
Your CSS looks okay. I'll note you can use padding: 5px 0 0;, with padding-left being automatically filled in.
You never use window in your plugin, so you don't need to pass it.
conf is shared across all instances of the plugin. Normally this is fine... but you're mutating conf, instead of using a separate variable to hold the defaults. This means that code like the below won't work as expected:

$(".thumbnails-carousel.not-centered").thumbnailsCarousel({ center: false });

// no need to specify because it's centered by default
$(".thumbnails-carousel.centered").thumbnailsCarousel();

Similarly, you are using a global shared "cache" which apparently holds a jQuery object created from a hard-coded string. In doing so, I am no longer able to:

dynamically generate the carousel
give the carousel my own class name
use more than one carousel in a non-buggy way

Instead, use instance variables. After all, you're providing $.fn.thumbnailsCarousel. When you call $(".thumbnails-carousel").thumbnailsCarousel(), $(".thumbnails-carousel") is passed in as this!
refreshOpacities() is oddly named. After going through it twice I now understand that it's just synchronising the active thumbnail with the active carousel image. Can you rename this function?
bindUiActions() is okay but the lowercase i is bugging me... although some may argue that this is perfectly fine (à la getElementById()...).
There's no point in explicitly specifying the empty object when calling .thumbnailsCarousel(), since $.extend(x, undefined) === x.

The following point becomes less relevant as you address the above points, mostly because I believe you should refactor the referenced code out anyway.

In init(), you use cache.$carouselContainer.find(".carousel-control"), which is plain silly because you already have cache.$controls.


Receive a list of names and return a list of users - HackerRank test So, few months ago I did a HackerRank test for a company. The problem was to create a function which receive a list of names and returns a list of unique usernames. For example:

For the list of names = ['john', 'john', 'tom', 'john']
The function must return: = ['john', 'john1', 'tom', 'john2']

My code was:

def username_system(u, memo={}, users=[]):
    copy_u = u.copy()
    try:
        name = copy_u[0]
    except IndexError:
        return users
    if name in memo.keys():
        memo[name] += 1
        username = name + str(memo[name])
        users.append(username)
        copy_u.remove(name)
        return username_system(copy_u, memo, users)
    else:
        username = name
        users.append(username)
        memo.update({name: 0})
        copy_u.remove(name)
        return username_system(copy_u, memo, users)
    return users

I'd like to know,  what could I improve in this code? I used memoization to make it faster. Also, I think this code is in O(n), but I'm not sure. Is that right?

I would warn you from using such an approach - it may lead to buggy and unexpected results.Consider the following situation:

names = ['john', 'john', 'tom', 'john']
res1 = username_system(names)
print(res1)    # ['john', 'john1', 'tom', 'john2']

lst = ['a']
res2 = username_system(names, users=lst)
print(res2)   # ['a', 'john3', 'john4', 'tom1', 'john5']

Looking at the 2nd print result ...
Using a mutable data structures as function default arguments is considered as a fragile approach - Python "memorizes"(retains) that mutable arguments content/value between subsequent function's calls. Furthermore, you use 2 of such.Though in some cases it may be viable for defining a specific internal recursive functions - I'd suggest a more robust and faster approach.

As you're mentioned about "memoization", "to make it faster", "O(N)" here's a deterministic profiling stats for your initial function:

import cProfile

# def username_system(...)
names = ['john', 'john', 'tom', 'john']
cProfile.run('username_system(names)')

Output:

         27 function calls (23 primitive calls) in 0.000 seconds

   Ordered by: standard name

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.000    0.000 <string>:1(<module>)
      5/1    0.000    0.000    0.000    0.000 test.py:3(username_system)
        1    0.000    0.000    0.000    0.000 {built-in method builtins.exec}
        4    0.000    0.000    0.000    0.000 {method 'append' of 'list' objects}
        5    0.000    0.000    0.000    0.000 {method 'copy' of 'list' objects}
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}
        4    0.000    0.000    0.000    0.000 {method 'keys' of 'dict' objects}
        4    0.000    0.000    0.000    0.000 {method 'remove' of 'list' objects}
        2    0.000    0.000    0.000    0.000 {method 'update' of 'dict' objects}

As you may observe even for that simple input list of 4 names username_system function is called 5 times.

Instead, we'll rely on supplementary collections.defaultdict object that conveniently provides the initial value.Then, traversing over a copy of initial user names only entries that occur more than 1 time (within a dict keys view) will be appended with incremented ordinal suffix:

from collections import defaultdict

def get_unique_usernames(user_names):
    d = defaultdict(int)
    uniq_unames = user_names[:]

    for i, name in enumerate(uniq_unames):
        if name in d:
           uniq_unames[i] += str(d[name])
        d[name] += 1

    return uniq_unames

if __name__ == "__main__":
    names = ['john', 'john', 'tom', 'john']
    print(get_unique_usernames(names))    # ['john', 'john1', 'tom', 'john2']

Comparison of Time execution performance:

initial setup:

from timeit import timeit

names = ['john', 'john', 'tom', 'john']

username_system function:

print(timeit('username_system(names)', 'from __main__ import names, username_system', number=10000))
0.027410352995502762

get_unique_usernames function:

print(timeit('get_unique_usernames(names)', 'from __main__ import names, get_unique_usernames', number=10000))
0.013344291000976227


Thread, handler and activity structure for a bouncing ball animation This is a custom MyActivity class with two custom views (bar and a ball):

public class MyActivity extends Activity {

    // set class variables
    RelativeLayout content;
    public Ball view;
    Bar bar;
    Handler handler;
    BounceLoop thread;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_my);
        content = (RelativeLayout) findViewById(R.id.content);
    }

    @Override
    public void onWindowFocusChanged(boolean hasChanged) {
        view = new Ball(this);
        bar = new Bar(this);
        Random rand = new Random();
        int min = 50;
        int max = content.getWidth();
        int randNum = rand.nextInt(max-min+1) + min;
        view.setPosition(randNum, min);
        bar.setY(content.getHeight() - 220);
        bar.setMaxWidth(content.getWidth());
        content.addView(bar);
        content.addView(view);
        thread = new BounceLoop();
        thread.start();
        handler = new Handler() {
            public void handleMessage(Message msg) {
                view.setPosition(msg.arg1,msg.arg2);
                view.invalidate();
            }
        };
    }
    @Override
    protected  void onDestroy() {
        super.onDestroy();
        content.removeAllViews();
    }
    @Override
    protected  void onResume() {
        super.onResume();
    }

    @Override
    protected void onPause() {
        super.onPause();
    }

    class BounceLoop extends Thread {
        public boolean running;
        int speed = 1;
        boolean reachedWidth, reachedHeight;
        int x = view.x,y = view.y, maxWidth = content.getWidth(), maxHeight = content.getHeight();
        int radius = 40;

        public void onPause() {
            running = false;
        }
        public boolean isCollision(int x, int y) {
            if( x > bar.x && x < bar.x + bar.getBarWidth()) {
                if(y >= bar.getY() - 50) {
                    return true;
                }
            }
            return false;
        }
        public void run() {

            running = true;
            Log.d("prevPos", String.valueOf(x + " : " + y));

            while(running) {
                if(reachedWidth) {
                    x -= speed;
                } else {
                    x += speed;
                }

                if(reachedHeight) {
                    y -= speed;
                } else {
                    y += speed;
                }

                if( x > maxWidth - radius) {
                    reachedWidth = true;
                } else if ( x < 0 + radius) {
                    reachedWidth = false;
                }

                if( y > maxHeight - radius || isCollision(x,y)) {
                    reachedHeight = true;
                } else if ( y < 0 + radius) {
                    reachedHeight = false;
                }
                try {
                    Thread.sleep(1);
                    Message msg = new Message();
                    msg.arg1 = x;
                    msg.arg2 = y;
                    handler.sendMessage(msg);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
    }
}

How do I re-construct it better and how can I put thread class outside? I really don't like having the thread class inside the activity class. The code seems to be working for now, but I really want to make it better! What can I do about it?

I don't have time to do a full review, but here are some pointers.

Don't use raw access to members (view.x), but always use getters/setters instead.  It has been a Java convention for about 15 years.  Also, you should declare all your members as private.
Don't do many assignements on a single line (int x = view.x,y = view.y, ...).  Again, it does work, but the Java convention is to have one assignment per line.
Don't have Ball being a member of MyActivity and MyActivity being a member of Ball.  That is spaghetti code.
Avoid using Thread in Android, or Java.  There are cleaner and less dangerous patterns for working with concurrent code. For Android, you should look into AsyncTask; for example this.  You can probably find a more appropriate pattern if you look harder.
I haven't touch Android for a while, but I don't think onWindowFocusChanged is the usual callback where everything is started.


Asking user input until a valid datetime is given (bloated try-except blocks) Originally asked this on SO but was redirected here.

I have a script that's asking the user to input a lot of dates, mostly in functions similar to the one below. These need to account for invalid date formats:

import numpy as np
import pandas as pd
import xlwings as xw
import datetime as dtt

def week_init():
        """Creates an initial week datetime and determines what the checking cutoff
        'beforeday' to be used in Stage 2 is."""
        week = input('Week to check: MM/DD/YYYY\n')
        switch = 1
        while switch == 1:
            try:
                week = dtt.datetime.strptime(week,'%m/%d/%Y') #turns input to a datetime
                switch = 0
            except ValueError: #couldn't parse as a date MM/DD/YYYY
                week = input('Unrecognized date format, please try again: MM/DD/YYYY\n')
        beforeday = (input('Check days before date (Press enter to use today): MM/DD/YYYY\n')
            or dtt.date.today())
        if (beforeday != dtt.date.today()):
            switch = 1
            while switch == 1:
                try:
                    beforeday = dtt.datetime.strptime(beforeday,'%m/%d/%Y')
                    switch = 0
                except ValueError: #couldn't parse as a date MM/DD/YYYY
                    beforeday = input('Unrecognized date format, please try again: MM/DD/YYYY\n')
        return week, beforeday

There are also functions that check an index for a given date, and have to deal with a given date not matching any of the indices:

def spotmap(week, bam, pt):
    """Maps the pivoted data for the chosen week to the BAM dataframe's SD column."""
    print('Mapping data to BAM... ',end=''),
    switch = 1
    while switch == 1:
        try:
            bam['SD'] = bam.index.to_series().map(pt.fillna(0)['len',week])
            switch = 0
        except KeyError: #date doesn't match any of the pivot table's columns
            print('Invalid date, please try again.')
            week = input('Week start date (Sunday): MM/DD/YYYY\n')
    print('Done')
    return bam

As the script has a lot of things to do after it gets these dates, I don't want it to crash when it has a problem, but it can't proceed without correct date inputs so I currently have it looping with those "switch" variables controlling when it is willing to move on with a valid date. As you can see, though, these try/except blocks are rapidly bloating the otherwise straightforward code. Is there a way to condense these things? Also, a better method than the switches?

https://stackoverflow.com/questions/9386592/repetitive-try-and-except-clauses

The answers here suggested decorators, but as far as I can tell from the documentation and pages like this, those are for wrapping functions, not for replacing interior code blocks. Also I'm not sure how useful they'd be, because the try/except blocks are mostly unique in what they're trying to accomplish (ie, which variable they're changing or whether they're checking against an index). I guess I'm really just hoping for some better general exception-handling syntax.

I will focus on the main question, i.e. how to decompose the code so that checks are kept, but the code does not get bloated with them.

Regarding the first part: I recommend to factor out the code asking for input, and split the initialization function into two separate ones. Something like this:

import datetime as dtt

def read_date(prompt, def_date=None):
          switch = 1
          while switch == 1:
                week = input(prompt) or def_date
                try:
                       week = dtt.datetime.strptime(week,'%m/%d/%Y')
                       switch = 0
                except ValueError:
                       print('Unrecognized date format, please try again\n')
          return week

def init_week():
    return read_date('Week to check: MM/DD/YYYY\n')

def init_before_day():
    return read_date('Check days before date (Press enter to use today): MM/DD/YYYY\n', '{0:%m/%d/%Y}'.format(dtt.date.today()))

def week_init():
    return [init_week(), init_before_day()]
    # alternatively (without the helpers): return [read_date(), read_date(dtt.date.today())]

For the second part, I suggest factoring out the mapping logic from inputting the date (or at least, the date should also be read in the function itself -- but receiving it as a parameter, and then reading it again is very messy IMHO). So this would look something like the following:

def spotmap_impl(week, bam, pt):
    bam['SD'] = bam.index.to_series().map(pt.fillna(0)['len',week])
    return bam

def spotmap(bam, pt):
    switch = 1
    while switch == 1:
        try:
            week = input('Week start date (Sunday): MM/DD/YYYY\n')
            bam = spotmap_impl(week, bam, pt)
            switch = 0
        except KeyError:
            print('Invalid date, please try again.')
    return bam;

Remarks:

I tried to leave the original names as much as possible, but I feel they could be improved (e.g. in read_date we are reading a date, which is then somehow used as a week; so I would call the internal variable something like input_date. Also instead of switch, I would use a name like success, and values True/False, instead of 1 and 0).
I could not test the second part, as I do not understand exactly what is the meaning and contents of bam and pt, but I hope you get the idea :) (Btw., these variable names might be improved too.)
You might want to add further validation code to the parameters of spotmap_impl. And then write unit tests for it, if you haven't done so yet ;)


Run multiple python codes and Check with answer data I wrote the code for my undergraduate TA jobs, to check assignment codes of students in Python class are correct or not.

Usage of the code is

$ python [test01.py[,test02 , ...] answer.dat grade.md

It will write grades (range 0~100) to a file called grade.md.

"""
Testing Assginments
by kidkkr
03-16-2017
"""

import os
import sys
from difflib import SequenceMatcher

class ResultChecker:
    """ Check python code result with answer data """
    __slots__ = '_srcFiles', '_ansFile', '_gradeFile', '_grade', '_ans'

    def __init__(self, srcFiles, ansFile, gradeFile):
        self._srcFiles = srcFiles
        self._ansFile = ansFile
        self._ans = open(self._ansFile, 'r').read()
        self._gradeFile = gradeFile
        self._grade = "# Grade File\n"

    def _run_code(self, src):
        print('-----------running ' + src + '..-----------')
        os.system('python ' + src + ' > result.swp')
        res = open('./result.swp', 'r').read()
        ratio = int(SequenceMatcher(None, res, self._ans).ratio() * 100)
        self._grade += src + ' ' + str(ratio) + '\n'
        print('result: ' + str(ratio))

    def check(self):
        for src in self._srcFiles:
            self._run_code(src)
        open(self._gradeFile, 'w').write(self._grade) # Write _grade to a file
        print('Checking has completed.')
        print(self._gradeFile + ":")
        print(self._grade)

def __main__():
    if len(sys.argv) < 2: # Check arguments
        raise ValueError('')

    srcFiles = sys.argv[1:-2]
    ansFile = sys.argv[-2]
    gradeFile = sys.argv[-1]

    ResultChecker(srcFiles, ansFile, gradeFile).check()

if __name__ == '__main__':
    __main__()

Here is something I would improve:

fix the variable naming - according to PEP8, use lower case and separate words with an underscore - e.g. _grade_file instead of _gradeFile
use argparse  module instead of manually reading from sys.argv - you will get the auto-generated command-line help for free and it is generally more readable, structured and scalable. As a bonus, you may avoid checking the length of the sys.argv and reading the "files" into variables. Something along these lines:

import argparse

def parse_arguments():
    parser = argparse.ArgumentParser()

    parser.add_argument('--source-files', nargs="+", type=str)
    parser.add_argument('--answer-file', type=argparse.FileType('r'))
    parser.add_argument('--grade-file', type=argparse.FileType('w'))

    return parser.parse_args()

Note the use of the special FileType argument type.

And, because of the use of nargs="+", you should change the way you pass python file names - instead of a comma-separated list, use space:

$ python --source-files test01.py test02.py --answer-file answer.dat --grade-file grade.md

use with context manager when opening files
try subprocess.Popen() instead of os.system() - with "popen" you may not need to temporarily write the result to a swap file and read the standard out and error directly
I understand that an underscore at the beginning of a variable name is a convention to mark "private variables", but, if the checker class would not be a part of a public-facing API, it would be okay to remove the underscores to improve on readability


Custom ListAdapter with ViewHolder Pattern I wrote this class for a custom list adapter for my list view. I use the view holder pattern to make the list view move smoother.

Could anyone advice tell me if I did something wrong, or some way to refine my code?

Example: defining my views. Is there way to make it faster?

import android.content.Context;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
import android.widget.BaseAdapter;
import android.widget.ImageView;
import android.widget.TextView;

import com.validate.R;

import java.util.List;

/**
 * Created by Mina on 30/06/2015.
 */
public class vault_listadapter extends BaseAdapter {
    private List<Vault_item_class> vault_items;
    private Context context;
    private LayoutInflater inflater;

    TextView merchantName_tv, voucherType_tv, voucherName_tv, voucherDetail_tv, voucherValue_tv,
            issueDate_tv, expiryDate_tv, voucherCode_tv;
    ImageView barcode_imageview;

    public void vault_listadapter(Context context, List<Vault_item_class> vault_items) {
        this.context = context;
        this.vault_items = vault_items;
        this.inflater = LayoutInflater.from(context);

    }

    @Override
    public int getCount() {
        return vault_items.size();
    }

    @Override
    public Vault_item_class getItem(int position) {
        return vault_items.get(position);
    }

    @Override
    public long getItemId(int position) {
        return position;
    }

    @Override
    public View getView(int position, View convertView, ViewGroup parent) {
        ViewHolder holder;

        if (convertView == null) {
            convertView = inflater.inflate(R.layout.vault_item, null, false);
            holder = new ViewHolder();

            holder.merchantName_tv = (TextView) convertView.findViewById(R.id.merchantName_id);
            holder.voucherName_tv = (TextView) convertView.findViewById(R.id.voucherName_id);
            holder.voucherType_tv = (TextView) convertView.findViewById(R.id.voucherType_id);
            holder.voucherDetail_tv = (TextView) convertView.findViewById(R.id.voucherDetails_id);
            holder.voucherValue_tv = (TextView) convertView.findViewById(R.id.voucherVaule_id);
            holder.issueDate_tv = (TextView) convertView.findViewById(R.id.issueDate_id);
            holder.expiryDate_tv = (TextView) convertView.findViewById(R.id.expiryDate_id);
            holder.voucherCode_tv = (TextView) convertView.findViewById(R.id.voucherCode_id);

            holder.barcode_imageview = (ImageView) convertView.findViewById(R.id.barcodeimg_id);

            convertView.setTag(holder);
        } else {
           holder = (ViewHolder) convertView.getTag();
        }

        return convertView;
    }

    static class ViewHolder {
        TextView merchantName_tv, voucherType_tv, voucherName_tv, voucherDetail_tv, voucherValue_tv,
                issueDate_tv, expiryDate_tv, voucherCode_tv;
        ImageView barcode_imageview;
    }
}

Here is a complex example for your case:

  List Adapter with ViewHolder Pattern

Note:

Never use a parameter as a variable (parameters shouldn't be modified!)
I like to use an underscore at the beginning of global fields (variables, objects) names to better show their scope.
You should make a class name without any special characters and beginning with an uppercase letter.
Try to get context from the closest possible parameter (as in my example from the parent of the getView() method).
The class constructor is defined without a return type (with the same name as the class name and any parameter if you desire)!
For later data changes in the adapter, you can define a method to add items to the list (as an example).


Converting imperative style user input menu to functional programming style I am currently trying to teach myself scala by rewriting projects from my intro CS class, which was taught in C++. In addition to just learning the syntax I am trying to gain understanding of how functional programming works and what is considered good functional programming style.

Below I've included both the original code, which is a simple while loop that continues looping until the user selects the 'quit' option, and my attempt at a functional equivalent. Have I done this correctly? What could be improved?

Original imperative version in C++:

int main()
{
  int option = -1;

  while (option != 3)
  {
    cout << fixed;
    cout << "Please select one of the following:" << "\n1 - one" <<
      "\n2 - two" << "\n3 - quit" << endl;
    cin >> option;
    if (option == 1)
    {
      cout << "selected 1" << endl;
    }
    else if (option == 2)
    {
      cout << "selected 2" << endl;
    }
    else if (option == 3)
    {
      cout << "selected quit.\n";
    }
    else
    {
      cout << "Sorry, that command is not recognized. ";
    }
  }
  return 0;
}

My attempt at a functional version in scala:

def menu(option: Int) {
  println("""|Please select one of the following:
             |  1 - one
             |  2 - two
             |  3 - quit""".stripMargin)
  if (option == 1) {
    println("selected 1")
    val opt = StdIn.readInt
    menu(opt)
  }
  else if (option == 2) {
    println("selected 2")
    val opt = StdIn.readInt
    menu(opt)
  }
  else if (option == 3) {
    println("selected quit")
  }
  else {
    println("Sorry, that command is not recognized")
  }
}

def main(args: Array[String]) {
  println("""|Please select one of the following:
             |  1 - one
             |  2 - two
             |  3 - quit""".stripMargin)
  val opt = StdIn.readInt
  menu(opt)
}

One of the Scala features you need to get used to using is match - it is your friend. One of your very best friends.

Also, the way you have defined menu is recursive, with no bounding. In this particular case, it's probably not an issue, but it is, in general a Really Bad Idea.

Finally, you have repeated the printing of the menu and reading of the option. In general, one should follow Don't Repeat Yourself.

So how about something like the code below. Note, one of the rare cases tailor made for do...while.

def menu(option: Int): Boolean = {
  option match {
    case 1 =>
      println("selected 1")
      true
    case 2 =>
      println("selected 2")
      true
    case 3 =>
      println("selected quit")
      false
    case _ => // the else case
      println("Sorry, that command is not recognized")
      true
  }
}

def readOption: Int = {
  println("""|Please select one of the following:
             |  1 - one
             |  2 - two
             |  3 - quit""".stripMargin)
  StdIn.readInt()
}

def main(args: Array[String]) {
  var opt = 0
  do {
    opt = readOption
  } while (menu(opt))
}

Alternately, if you wanted to something more functional, you could change menu to something like this:

val actionMap = Map[Int, () => Boolean](1 -> handleOne, 2 -> handleTwo, 3 ->handleThree)

def handleOne(): Boolean = {
  println("selected 1")
  true
}

def handleTwo(): Boolean = {
  println("selected 2")
  true
}

def handleThree(): Boolean = {
  println("selected quit")
  false
}

def menu(option: Int): Boolean = {
  actionMap.get(option) match {
    case Some(f) => f()
    case None =>
      println("Sorry, that command is not recognized")
      false
  }
}

Where actionMap maps each Int to a function that takes no parameters and returns a Boolean. In menu, we use option as the key into the map and then call the result (if there is one).


Getting currency data out of an array I have a button dropdown that you can select different currencies from. When you select one, it replaces the button value with that currency type, and displays the currency symbol in a span in front of an input.

I have done some refactoring where I now have just one function getCurrency() that accepts parameters and inserts the proper values, but in here $(".dropdown-menu li").click( function(){...} I have 3 different if else statements. I know there has to be a way to do this by looping through my currencyData array and use this to target a specific element, but I cannot figure it out.

I want to make this more modular so it can scale up. If I had 30 different currencies, this would not work. I would have to change too many lines of code, where I should just be able to add another currency to the array.

It is live, you can view it here.

JS

var currencyData = [
  {
    "currency": "USD",
    "symbol" : "$"
  },
  {
    "currency": "EUR",
    "symbol" : "€"
  },
  {
    "currency": "JPY",
    "symbol" : "¥"
  }
];

/*
 * Inserts
 list of values into the currency dropdown
*/
$.each( currencyData, function( key, val ) {

  var $li = $("
" +
    val.currency + "
");

  $(".dropdown-menu").append( $li );
 });

/*
 * Generate currency symbol and type and swap them depending on which is clicked
*/
$(".dropdown-menu li").click( function() {

 if ( this.textContent === currencyData[0].currency ) {
    getCurrency(currencyData[0].currency, currencyData[0].symbol);

 } else if ( this.textContent === currencyData[1].currency ) {
    getCurrency(currencyData[1].currency, currencyData[1].symbol);

 } else if ( this.textContent === currencyData[2].currency ) {
    getCurrency(currencyData[2].currency, currencyData[2].symbol);
 };
});

/*
 * Swap currency type and symbol
*/
function getCurrency(currency, symbol) {
  // inserts visible currency symbol to form
  var $span = $(" " + symbol + "");

  // hidden input to save currency symbol
  var $input = $("<input type='text' class='js-hidden-currency-symbol' value=" +
   symbol + " style='display:none' name='currencySymbol'>");

  $('.js-currency-symbol').remove();
  $('.js-currency-btn').text( currency );
  $('.js-input-amount-prepend').append( $span );

  $('.js-hidden-currency-symbol').remove();
  $('.js-currency-btn-group').append( $input );
};

HTML

      Amount:

      <input type="text" required class="js-payment-amount-input payment-amount-input" name="amount" placeholder="enter amount">

        <button type="button" class="btn js-currency-btn currency-btn">Currency</button>
        <button type="button" class="btn dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">

          Toggle Dropdown
        </button>

Work with indices instead of elseif statements

You should write a function that identifies the index of this.textContent in the currencyData array. Why not try something like this:

function myIndexOf(o) {
    for (var i = 0; i < currencyData.length; i++) {
        if (currencyData[i].currency == o) {
            return i;
        }
    }
    return -1;
}

Taken from this Stackoverflow answer.

Then you can run:

currentIndex = myIndexOf(this.textContent)
getCurrency(currencyData[currentIndex].currency, currencyData[currentIndex].symbol);

And it's entirely extensible.

Avoid unnecessary HTML updates

Seems like you can reduce $span to only update the innerHTML of a span element rather than reinitializing your span element each time you append it to $('.js-input-amount-prepend'). This would slightly shorten getCurrency