Conversion of datetime strings in PHP pages to user's timezone I am working on a PHP/MySQL application that uses a different time zone from the server it runs on, and neither of the timezones are UTC.  I'm converting displayed times to be in the user's timezone, and am wondering whether my approach is sensible.  I would love any feedback you might have.

Firstly, the application's current time is written to the output HTML page as a meta value:

<meta name="date" content="<?php echo date('Y-m-d H:i:s') ?>" />

And, every instance of a datetime string is wrapped thus:

<?php echo $data->datetime_column ?>

Then, after the page is loaded, the following JQuery gets the application time, the user time, figures out the difference between these, and adds that difference to the datetime data:

$("span.datetime-convert").each(function(){
    var serverTime = parseDatetime($("meta[name='date']").attr("content"));
    var userTime = new Date();
    var userOffset = new Date(userTime.getTime() - serverTime.getTime());
    var timeToConvert = parseDatetime($(this).text());
    var convertedTime = new Date(timeToConvert.getTime() + userOffset.getTime());
    $(this).text(convertedTime.toLocaleString());
});
function parseDatetime(value) {
    var a = /^(\d{4})-(\d{2})-(\d{2}) (\d{2}):(\d{2}):(\d{2})$/.exec(value);
    if (a) {
        return new Date(+a[1], +a[2] - 1, +a[3], +a[4], +a[5], +a[6]);
    }
    return null;
}

What am I doing wrong?

I would think it would be easier and more reliable to use timezone offsets rather than exact server time vs. client time. You can get the client-side offset using new Date().getTimezoneOffset() which returns the offset in minutes from UTC. When the timezone is +2 then the offset is -120.

PHP

<meta name="timezoneoffset" content="<?php echo date('P') ?>" />

HTML

<meta name="timezoneoffset" content="+0200" />

JavaScript

var serverTimeZone= $("meta[name='timezoneoffset']").attr("content").substring(0,3);
// getTimezoneOffset returns (client timezone * -60).
// With timezones the the formula would be (clientTz - serverTz) * 60 * 60000
var timezoneOffset = (new Date().getTimezoneOffset() + (serverTimeZone * 60)) * 60000;
$("span.datetime-convert").each(function(){
    var date = parseDatetime($(this).text());
    if(date) { // If date is not wellformed the script would crash without this guard
      var offsetDate = new Date(date.getTime() + timezoneOffset);
      $(this).text(offsetDate .toLocaleString());
    }
});
function parseDatetime(value) {
    var a = /^(\d{4})-(\d{2})-(\d{2}) (\d{2}):(\d{2}):(\d{2})$/.exec(value);
    if (a) {
        return new Date(+a[1], +a[2] - 1, +a[3], +a[4], +a[5], +a[6]);
    }
    return null;
}

Also there's no need to calculate the offset for each individual date. It can be actually a bit misleading when the offset is slighlty different for each date.

http://jsfiddle.net/xXVPC/1/

More functional:

var serverTimeZone= $("meta[name='timezoneoffset']").attr("content").substring(0,3);
// getTimezoneOffset returns (client timezone * -60).
// With timezones the the formula would be (clientTz - serverTz) * 60 * 60000
var timezoneOffset = (new Date().getTimezoneOffset() + (serverTimeZone * 60)) * 60000;
$("span.datetime-convert").each(function(){
    var e = $(this);
    parseDatetime(e.text(), new function(date) {
      var offsetDate = new Date(date.getTime() + timezoneOffset);
      e.text(offsetDate.toLocaleString());
    });
});
function parseDatetime(value, callback) {
    var a = /^(\d{4})-(\d{2})-(\d{2}) (\d{2}):(\d{2}):(\d{2})$/.exec(value);
    if (a) {
        callback(new Date(+a[1], +a[2] - 1, +a[3], +a[4], +a[5], +a[6]));
    }
}


Retrieve rich text from GridFS with Java I am writing a storage for articles and I have decided to use GridFS from MongoDB. I have found and tested some code, but I am not sure that it is a best one. Could you advice me how to make it better?

private GridFS gridFs;

public String getPostContent(String name) {
    GridFSDBFile file = gridFs.findOne(name + ".html");
    if (file == null) {
        return "There is no text for this article.";
    }
    return getStringFromInputStream(file.getInputStream());
}

private static String getStringFromInputStream(InputStream is) {
    BufferedReader bufferedReader = null;
    StringBuilder builder = new StringBuilder();

    String line;
    try {
        bufferedReader = new BufferedReader(new InputStreamReader(is));
        while ((line = bufferedReader.readLine()) != null) {
            builder.append(System.getProperty("line.separator")).append(line);
        }
    } catch (IOException e) {
        e.printStackTrace();
    } finally {
        if (bufferedReader != null) {
            try {
                bufferedReader.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }
    return builder.toString();
}

I will appreciate any suggestions even regarding chosen approach.

Since you are already on Java 7, you can use try-with-resources on your BufferedReader for safe and efficient handling of the underlying I/O resource.

Also, if all you are interested in is to read a file contents as a single String, you can consider reading them into a String, and then there appears to be three simple ways using either Guava, Apache Commons IO or Spark (web framework) libraries:

// Guava
String content = new String(ByteStreams.toByteArray(is));
// Apache Commons IO
String content = IOUtils.toString(is, Charset.defaultCharset());
// Spark (web framework)
String content = IOUtils.toString(is);

Where applicable, it's recommended to specify the Charset explicitly, instead of relying on the default.

One difference with this approach is that newlines aren't normalized to System.getProperty("line.separator") like what you have done here. If this is crucial for your usage, then a slight refactoring can be:

private static String getStringFromInputStream(InputStream is) {
    try (InputStreamReader source = new InputStreamReader(is);
            BufferedReader bufferedReader = new BufferedReader(source)) {
        StringBuilder builder = new StringBuilder();
        // using for-loop for nicer scoping of 'line' variable
        for (String line = bufferedReader.readLine(); line != null;
                line = bufferedReader.readLine()) {
            if (builder.length() > 0) {
                builder.append(System.lineSeparator());
            }
            builder.append(line);
        }
        return builder.toString();
    } catch (IOException e) {
        // recommended to do something about it instead of just printStackTrace()
        // e.printStackTrace();
    }
}

Since you also suggest giving ideas about Java 8, you can use StringJoiner as a slightly better replacement - there is no need to handle the appending of System.lineSeparator() manually:

private static String getStringFromInputStream(InputStream is) {
    try (InputStreamReader source = new InputStreamReader(is);
            BufferedReader bufferedReader = new BufferedReader(source)) {
        StringJoiner builder = new StringJoiner(System.lineSeparator());
        for (String line = bufferedReader.readLine(); line != null;
                line = bufferedReader.readLine()) {
            joiner.add(line);
        }
        return joiner.toString();
    } catch (IOException e) {
        // do something
    }
}


Count number of each char in a String I know there is a simpler way of doing this, but I just really can't think of it right now. Can you please help me out?

String sample = "hello world";
char arraysample[] = sample.toCharArray();
int length = sample.length();

//count the number of each letter in the string
int acount = 0;
int bcount = 0;
int ccount = 0;
int dcount = 0;
int ecount = 0;
int fcount = 0;
int gcount = 0;
int hcount = 0;
int icount = 0;
int jcount = 0;
int kcount = 0;
int lcount = 0;
int mcount = 0;
int ncount = 0;
int ocount = 0;
int pcount = 0;
int qcount = 0;
int rcount = 0;
int scount = 0;
int tcount = 0;
int ucount = 0;
int vcount = 0;
int wcount = 0;
int xcount = 0;
int ycount = 0;
int zcount = 0;

for(int i = 0; i < length; i++)
{
    char c = arraysample[i];
    switch (c)
    {
        case 'a':
            acount++;
            break;
        case 'b':
            bcount++;
            break;
        case 'c':
            ccount++;
            break;
        case 'd':
            dcount++;
            break;
        case 'e':
            ecount++;
            break;
        case 'f':
            fcount++;
            break;
        case 'g':
            gcount++;
            break;
        case 'h':
            hcount++;
            break;
        case 'i':
            icount++;
            break;
        case 'j':
            jcount++;
            break;
        case 'k':
            kcount++;
            break;
        case 'l':
            lcount++;
            break;
        case 'm':
            mcount++;
            break;
        case 'n':
            ncount++;
            break;
        case 'o':
            ocount++;
            break;
        case 'p':
            pcount++;
            break;
        case 'q':
            qcount++;
            break;
        case 'r':
            rcount++;
            break;
        case 's':
            scount++;
            break;
        case 't':
            tcount++;
            break;
        case 'u':
            ucount++;
            break;
        case 'v':
            vcount++;
            break;
        case 'w':
            wcount++;
            break;
        case 'x':
            xcount++;
            break;
        case 'y':
            ycount++;
            break;
        case 'z':
            zcount++;
            break;
        }
}
System.out.println ("There are " +hcount+" h's in here ");
System.out.println ("There are " +ocount+" o's in here ");

Oh woah! xD It's just.. woah! What patience you have to write all those variables.

Well, it's Java so you can use a HashMap.

Write something like this:

String str = "Hello World";
int len = str.length();
Map<Character, Integer> numChars = new HashMap<Character, Integer>(Math.min(len, 26));

for (int i = 0; i < len; ++i)
{
    char charAt = str.charAt(i);

    if (!numChars.containsKey(charAt))
    {
        numChars.put(charAt, 1);
    }
    else
    {
        numChars.put(charAt, numChars.get(charAt) + 1);
    }
}

System.out.println(numChars);

We do a for loop over all the string's characters and save the current char in the charAt variable
We check if our HashMap already has a charAt key inside it

If it's true we will just get the current value and add one.. this means the string has already been found to have this char.
If it's false (i.e. we never found a char like this in the string), we add a key with value 1 because we found a new char

Stop! Our HashMap will contain all chars (keys) found and how many times it's repeated (values)!


Fibonacci generator with Golang This is my Fibonacci generator:

package main

import "fmt"

func main() {
    for i, j := 0, 1; j < 100; i, j = i+j,i {
        fmt.Println(i)
    }
}

It's working, but I don't know how I can improve it, so I'd like more expert approaches to solving it.

Your code is quite nice, and can't really be improved. However, let's put the “generator” back into the code.

Go has channels which can be used to write elegant generators/iterators. We spawn of a goroutine that fills the channel with the fibonacci sequence. The main thread then takes as many fibonacci numbers as it needs.

So let's write a function fib_generator that returns a channel to the fibonacci sequence:

func fib_generator() chan int {
  c := make(chan int)

  go func() {
    for i, j := 0, 1; ; i, j = i+j,i {
        c <- i
    }
  }()

  return c
}

We return a chan int. Here, we use an unbuffered channel. You may want to introduce a bit of buffering, e.g. make(chan int, 7).

Next, we spawn a goroutine. This contains your code, but instead of printing the numbers, we fill the channel with them. Note that the generator does not have a termination condition.

The unusual syntax

go func() { ... }()

is the standard way to start a goroutine. Because the function literal is a closure over the channel c, we can run multiple generators concurrently.

Our main will look like

func main() {
    c := fib_generator()
    for n := 0; n < 12 ; n++ {
        fmt.Println(<- c)
    }
}

That is, we create a new generator, and then pull the first 12 values from the channel. We cannot write for i := range c { ... }, because the fibonacci generator does not terminate.

As I said, we can have multiple generators concurrently:

func main() {
    c1 := fib_generator()
    c2 := fib_generator()

    // read first 12 numbers from 1st channel
    for n := 0; n < 10 ; n++ { fmt.Print(" ", <- c1) }
    fmt.Println()

    // read first 12 numbers from 2nd channel. The same.
    for n := 0; n < 10 ; n++ { fmt.Print(" ", <- c2) }
    fmt.Println()

    // read next   5 numbers from 1st channel.
    for n := 0; n <  5 ; n++ { fmt.Print(" ", <- c1) }
    fmt.Println()
}

Output:

 0 1 1 2 3 5 8 13 21 34
 0 1 1 2 3 5 8 13 21 34
 55 89 144 233 377


Reuse object in model in Rails Using Rails 3.2. I have the following code:

# photo.rb
class Photo < ActiveRecord::Base
  after_create :associate_current_user
  after_save :increase_user_photos_count
  after_destroy :decrease_user_photos_count

  private

  def associate_current_user
    current_user = UserSession.find.user
    self.user_id = current_user.id
  end

  def increase_user_photos_count
    current_user = UserSession.find.user
    User.increment_counter(:photos_count, current_user.id)
  end

  def decrease_user_photos_count
    current_user = UserSession.find.user
    User.decrement_counter(:photos_count, current_user.id)
  end
end

Before a new record is created, it searches for the current_user. This is alright if it's just 1 new record at a time. But if there are 100 records to be created, it's gonna search for the same current_user 100 times. There is definitely performance issue.

How should I refactor this so that the app takes the first query result and reuse it for the next 99 times?
After refactoring, does this affect other users who are also uploading their photos using their accounts?

Note: For some reasons, I can't use the counter_cache.

Remove the callback, and call that code at the level where it's used best.

I.e.:

When you create a batch of records:

#find and cache user
user = UserSession.find.user
# create your 100 records however you want... this is a dumb example
100.times { Post.create!(user: user) }

When you're in a controller, you want to do the same thing, except you're just creating one post.

user = UserSession.find.user
Post.create!(user:user, some:other, params:go_here)

Now that I've written it, it appears if you wanted to DRY up this example, you could wrap it in some class like this:

class CreatesPosts
  def self.create(num_records = 1, user = UserSession.find.user)
    num_records.times { Post.create!(user:user) }
    # might need to tweak to vary up the Post attributes
  end
end


Verify if meeting datetime ranges overlap in an IEnumerable list of meetings I have an assignment where I need to find out if any of the meetings in a list of meeting class object have overlapping datetime ranges. What I have so far is this:

public class Meeting
{
    public DateTime Start { get; private set; }
    public DateTime End { get; private set; }

    public Meeting(DateTime start, DateTime end)
    {
        this.Start = start;
        this.End = end;
    }
}

public static class StaffMeetings
{
    public static bool DoesNotOverlap(IEnumerable<Meeting> meetings)
    {
        DateTime start = new DateTime();
        DateTime end = new DateTime();
        foreach (Meeting meeting in meetings)
        {
            if (start != DateTime.MinValue && end != DateTime.MinValue)
            {
                if (start < meeting.End && end > meeting.Start)
                {
                    return false;
                }
            }
            start = meeting.Start;
            end = meeting.End;
        }
        return true;
    }

    public static void Main(string[] args)
    {
        var format = System.Globalization.CultureInfo.InvariantCulture.DateTimeFormat;

        Meeting[] meetings = new Meeting[]
        {
        new Meeting(DateTime.Parse("1/1/2015 20:00", format), DateTime.Parse("1/1/2015 21:30", format)),
        new Meeting(DateTime.Parse("1/1/2015 21:30", format), DateTime.Parse("1/1/2015 23:00", format)),
        new Meeting(DateTime.Parse("1/1/2015 23:10", format), DateTime.Parse("1/1/2015 23:30", format))
        };

        Console.WriteLine(StaffMeetings.DoesNotOverlap(meetings));
    }
}

Using the provided data set, I get the correct result but as I add more meetings, my results start to fail and performance drops.

I feel that the problem is in how I am iterating the list. However, if I sort the list first, I can most likely improve reliability but my performance would probably drop. I feel that there should be an algorithm for this, but so far, I have not had any luck finding one.

Sorting is what actually makes it work but if you also want it to be more linq-ish and more solid then you should change the following things:

move the overlap-logic into another method
change the method to return an IEnumerable<Meeting[]> for meetings that overlap
use Any to find out whether any meeting overlap
make both methods extensions

Example:

public static IEnumerable<Meeting[]> Overlappings(this IEnumerable<Meeting> meetings)
{
    var last = (Meeting)null;
    foreach (var meeting in meetings.OrderBy(m => m.Start))
    {
        if (last != null && meeting.OverlapsWith(last))
        {
            yield return new [] { last, meeting };
        }
        last = meeting;
    }
}

public static bool OverlapsWith(this Meeting x, Meeting y)
{
    return x.Start < y.Start ? x.End > y.Start : y.End > x.Start;
}

Gets overlapping meetings:

var overlappingMeetings = meetings.Overlappings();

or checks if there are any. It stops at the first overlapping pair found:

var meetingsOverlap = meetings.Overlappings().Any();

with these new extensions you are more flexible because you can easily check any two meetings overlap and it's easier to undestand than DoesNotOverlap. Usually it's better to write positive conditions and negate them if necessary rather than the other way around.

But even this could be further improved by overloading the two < and > operators:

public static bool operator <(Meeting x, Meeting y)
{
    return x.End < y.Start;
}

public static bool operator >(Meeting x, Meeting y)
{
    return x.End > y.Start;
}

and making the OverlapsWith method still more natural:

public static bool OverlapsWith(this Meeting x, Meeting y)
{
    return x.Start < y.Start ? x < y : y > x;
}


Binary file validity in C++/C++11: checking a binary header I have some code for checking a binary header to see if a binary file is meant to be read by a certain program, and I could use some extra eyes to review it.

Assume the following typedefs:

namespace Types
{
    using Byte = std::uint8_t;      // From <cstdint>
    using UInt32 = std::uint32_t;
    using String = std::string;     // From <string>
}

// This is a vector of bytes loaded from a binary file.
using ByteBuffer = std::vector<Types::Byte>;    // From <vector>

Also assume the following constants:

namespace Constants
{
    const Types::String     G_BINARY_HEADER_STRING  = "A2FORMAT";
    const Types::Byte       G_BINARY_MAJOR_VERSION  = 0x00;
    const Types::Byte       G_BINARY_MINOR_VERSION  = 0x01;
    const Types::UInt32     G_BINARY_HEADER_SIZE    = 14;
}

Here is the binary-header-validating code:

bool isBufferValid (ByteBuffer &a_buffer)
{
    // Get the size of the buffer. Make a cursor here, too.
    Types::UInt32 l_size = a_buffer.size();
    Types::UInt32 l_cursor = 0;

    // Check to see if the size exceeds 14 bytes.
    if (l_size <= Constants::G_BINARY_HEADER_SIZE)
        return false;

    // Bytes #1 to #8 are the characters in the binary header string.
    for (l_cursor = 0; l_cursor < Constants::G_BINARY_HEADER_STRING.size(); ++l_cursor)
    {
        if (a_buffer.at(l_cursor) != Constants::G_BINARY_HEADER_STRING.at(l_cursor))
        {
            return false;
        }
    }

    // Byte #9 indicates the engine's major version.
    //
    // If the byte found doesn't match the current major version, then it is invalid.
    if (a_buffer.at(8) != Constants::G_BINARY_MAJOR_VERSION)
        return false;

    // Byte #10 indicates the engine's minor version.
    //
    // If this byte given is greater than the current minor version, then it is invalid.
    if (a_buffer.at(9) > Constants::G_BINARY_MINOR_VERSION)
        return false;

    // Bytes #11 through #14 make up a 32-bit unsigned integer that indicates how many
    // bytes of data reside after the binary header.
    //
    // These bytes are arranged in Little Endian Notation.
    Types::UInt32 l_sizeOfData = 0;
    for (Types::UInt32 i = 0, j = 10; j < 14; i++, j++)
        l_sizeOfData |= a_buffer.at(j) << (i * 8);

    // If the amount of bytes in the buffer, after the header, do not match the number found
    // in the integer above, then the buffer is invalid.
    if (l_size - Constants::G_BINARY_HEADER_SIZE != l_sizeOfData)
        return false;

    // Remove the header from the buffer, because it has been confirmed valid, and is
    // therefore ready to use.
    a_buffer.erase(a_buffer.begin(), a_buffer.begin() + Constants::G_BINARY_HEADER_SIZE);

    return true;
}

Is there anything I can do to make this code more efficient? Are there also any steps I can take to make this validation more effective?

The most unforgivable fault with this function is that calling isValidBuffer() also erases the buffer! That completely violates the Principle of Least Surprise. If anything, the parameter should be const.


Code First Entity Framework Is the following good design for doing entity framework code first? What am I missing for a production system? I haven't included all my code, just a snapshot...

My application, doesn't update the database; it just reads it.

public class Document
{
    [Key, ScaffoldColumn(false)]
    public Int32 DocumentID { get; set; }

    [ScaffoldColumn(false)]
    public Boolean? Status { get; set; }

    [Display(Name = "Document Text"), DataType(DataType.MultilineText)]
    public String DocumentText { get; set; }

    [ScaffoldColumn(false), DataType(DataType.Url)]
    public String DocumentFolderPath { get; set; }

    [ScaffoldColumn(false), DataType(DataType.Url)]
    public String DocumentJSONPath { get; set; }

    [ForeignKey("DocumentID")]
    public virtual DocumentMetadata Metadata { get; set; }

    [ForeignKey("DocumentID")]
    public virtual ICollection<Page> Pages { get; set; }
}

public class Page
{
    [Key, ScaffoldColumn(false)]
    public Int32 PageID { get; set; }

    [Required, Display(Name = "Page"), DataType(DataType.Text)]
    public Int32 PageNumber { get; set; }

    [Required, ScaffoldColumn(false)]
    public Int32 DocumentID { get; set; }

    [ScaffoldColumn(false), DataType(DataType.ImageUrl)]
    public String ImagePath { get; set; }

    [Required, Display(Name = "Page Text"), DataType(DataType.MultilineText)]
    public String PageText { get; set; }

    [NotMapped]
    public String HighlightedText { get; set; }

    [ForeignKey("PageID")]
    public virtual ICollection<Word> Words { get; set; }
}

public class ArchiveDatabaseInitializer : DropCreateDatabaseIfModelChanges<ArchiveContext>
{
    protected override void Seed(ArchiveContext context)
    {
        try
        {
            GetDocuments().ForEach(d => context.Documents.Add(d));
            GetDocumentMetatdata().ForEach(d => context.DocMetadata.Add(d));
            GetPages().ForEach(d => context.Pages.Add(d));
            GetWords().ForEach(d => context.Words.Add(d));
            context.SaveChanges();
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }

    private static List<Document> GetDocuments()
    {
        var documents = new List<Document> {
            new Document
            {
                DocumentID = 1,
                DocumentFolderPath = @"Doc1",
                DocumentJSONPath = @"Doc1.json",
                Status = true
            },
            new Document
            {
                DocumentID = 2,
                DocumentFolderPath = @"Doc2",
                DocumentJSONPath = @"Doc2.json",
                Status = true
            }
        };
        return documents;
    }

public class ArchiveContext : DbContext
{
    public ArchiveContext()
        : base("archiveDB")
    {
    }
    public DbSet<Document> Documents { get; set; }
    public DbSet<DocumentMetadata> DocMetadata { get; set; }
    public DbSet<Page> Pages { get; set; }
    public DbSet<Word> Words { get; set; }
}

If you chose the code-first approach, I presume you don't have to deal with the constraints of an existing database. If that assumption is correct, then I'd say you've done it the hard way.

Convention Over Configuration

Entity Framework can infer keys (primary and foreign) from the names of your entity members; your code isn't leveraging this formidable capability.

For a greenfield project, I like to start with a base entity type:

public abstract class EntityBase
{
    public int Id { get; set; } // inferred PK
    public DateTime DateInserted { get; set; }
    public DateTime? DateUpdated { get; set; }
}

Now I can derive, say, Document from that class:

public class Document : EntityBase
{
    public bool? Status { get; set; }
    public string DocumentText { get; set; }
    // ...

    public virtual DocumentMetadata Metadata { get; set; } // inferred FK
}

The PK is inferred from the inherited Id property (or [TypeName]Id), and the mere mention of a virtual property referencing another entity type is enough for EF to understand you want a FK there.

I'll pause here to mention that I was somewhat thrown off by your non-usage of the C# language aliases for System.String, System.Int32 and System.Boolean. They're typically written as string, int and bool.

You mention you're only reading from the database. This sounds like you're going code-first with an existing database. It's certainly possible, but the best way to do this in my opinion, is to reverse-engineer the database into entity classes - then you get the best of both worlds, and the generated code can teach you a lot about how EF works.

I think the presence of DisplayAttribute and NotMappedAttribute smells like you're possibly using your entities as both data entities and presentation / ViewModels. I think I would separate these concerns and create separate classes for presentation purposes,  where HighlightedText means something relevant.

Also note that relying on a declarative DisplayAttribute will make it quite a pain to localize the application, if you ever need to do that in the future; you'll want these things resolved at run-time, accounting for Thread.CurrentThread.CurrentUICulture.

The naming is overall ok, except I don't like the Status column. It's just too vague of a name. What do the null, true and false values mean? The name should convey that meaning. Also I wouldn't prefix most of Document's members with the word "Document", and ID should follow the PascalCasing convention and be Id.


Efficiently performing multiple nested grouping and mapping in Javascript I have an array of log objects with log_types and dates, like this:
[
  {
    log_type: 'eventOne',
    date: '2020-08-27T00:00:00+00:00'
  },
  {
    log_type: 'eventOne',
    date: '2020-08-27T00:00:00+00:00'
  },
  {
    log_type: 'eventTwo',
    date: '2020-08-27T00:00:00+00:00'
  },
  {
    log_type: 'eventTwo',
    date: '2020-08-27T00:00:00+00:00'
  },
  {
    log_type: 'eventTwo',
    date: '2020-08-27T00:00:00+00:00'
  },
  {
    log_type: 'eventOne',
    date: '2020-08-27T00:00:00+00:00'
  },
]

For charting purposes, I need group all these logs by their log_types. Then I need to take all the logs for each log type, and group them by date. These will be formatted where t is the date and y is the count of that date occurring. Like this:
{
  eventOne: [
    { x: 2020-08-21T04:00:00.000Z, y: 0 },
    { x: 2020-08-22T04:00:00.000Z, y: 6 },
    { x: 2020-08-23T04:00:00.000Z, y: 0 },
    { x: 2020-08-24T04:00:00.000Z, y: 16 },
    { x: 2020-08-25T04:00:00.000Z, y: 0 },
    { x: 2020-08-26T04:00:00.000Z, y: 0 },
    { x: 2020-08-27T04:00:00.000Z, y: 22 }
  ],
  eventTwo: [
    { x: 2020-08-21T04:00:00.000Z, y: 0 },
    { x: 2020-08-22T04:00:00.000Z, y: 0 },
    { x: 2020-08-23T04:00:00.000Z, y: 1 },
    { x: 2020-08-24T04:00:00.000Z, y: 0 },
    { x: 2020-08-25T04:00:00.000Z, y: 0 },
    { x: 2020-08-26T04:00:00.000Z, y: 0 },
    { x: 2020-08-27T04:00:00.000Z, y: 17 }
  ]
}

So in plain english the goal is to get the number of events that were logged for each day, for each log type.
I've accomplished this, but it is a series of mangled lodash functions to where any sense of readability or efficiency has gone out the window.
// First group by log_type
const logTypes = _.groupBy(logs, 'log_type');

const mappedLogs = {};
// Iterate through each log type
_.forOwn(logTypes, (value, key) => {
  const logDates = _.chain(value)
     // For each log type, group by date
     .groupBy('date')
     // Then map each log to the correct format
     .map((logValue, logKey) => ({ t: logKey, y: logValue.length }))
     .value();
  // Once we get the final mapped date values, assign them to the final object
  mappedLogs[key] = logDates;
});

I'm really hoping to find a way to do this more efficiently, I'm having trouble determining the efficiency of this current algorithm but I know the initial _groupBy touches each log O(n), the second _forOwn plus _groupBy I believe is another O(n), then the final mapping to correct format would own another O(n).
This is an application where there could be many thousands of logs so that is not an ideal efficiency.

I don't see what lodash has to offer here.  With modern JavaScript it can be written quite tersely thus:
const events = data.reduce((events, { log_type, date }) => {
  const event = (events[log_type] || (events[log_type] = []));
  const match = event.find(({ x }) => x == date);
  match ? match.y++ : event.push({ x: date, y: 1 });
  return events;
}, { });

The first line inside the .reduce method creates the event array if it doesn't exist.
The second line finds the element with a matching date.
The third line, if a matching date was found, increments its y property.  Else, it adds a new element with the date.
For example:

const data = [
  {
    log_type: 'eventOne',
    date: '2020-08-27T20:00:00+00:00'
  },
  {
    log_type: 'eventOne',
    date: '2020-08-27T20:00:00+00:00'
  },
  {
    log_type: 'eventTwo',
    date: '2020-08-27T00:00:00+00:00'
  },
  {
    log_type: 'eventTwo',
    date: '2020-08-27T00:00:00+00:00'
  },
  {
    log_type: 'eventTwo',
    date: '2020-08-27T40:00:00+00:00'
  },
  {
    log_type: 'eventOne',
    date: '2020-08-27T00:00:00+00:00'
  },
];

const events = data.reduce((events, { log_type, date }) => {
  const event = (events[log_type] || (events[log_type] = []));
  const match = event.find(({ x }) => x == date);
  match ? match.y++ : event.push({ x: date, y: 1 });
  return events;
}, { });

console.log(events);


PermCheck Codility with time complexity of O(N) I have this solution for Codility's PermCheck problem.  In summary, the task is to check whether array A contains each number in \$1 \ldots N\$ exactly once.

I got 100% for it but I got a time complexity of \$O(N * log(N))\$ or \$O(N)\$. How could I make this code \$O(N)\$? Could you also give a brief description of what makes code \$O(N)\$?

Array.Sort(A);
if(A[0] == 1 && A.Length == 1) return 1;
if(A[0] != 1) return 0;

int n = 0;
for(int i = 0; i < A.Length; i++)
{
    if(i < A.Length - 1)
    {
        if(A[i+1] == A[i] + 1)
        {
            n += 1;
        }
        else
        {
            return 0;
        }
    }
}
return 1;

This code involves sorting an array of \$N\$ elements, so it has \$O(N * log N)\$ time complexity. To achieve \$O(N)\$ time complexity, you need to avoid sorting input data. Instead, you can create an array of \$N\$ bools to check that each number from \$1\$ to \$N\$ is present in the input array.


My simple calculator I'm new to Java and coding in general. I'm about a week or two into learning Java, and I decided to make this simple four function calculator. Any feedback/tips?

import java.util.Scanner;

class Main {

public static void main(String[] args) {
    Scanner input = new Scanner(System.in);

    System.out.println("What is your first number?");
    float num1 = input.nextFloat();
    System.out.println("What is your second number?");
    float num2= input.nextFloat();
    System.out.println("What operation would you like to perform?");
    String Operation = input.next();
    switch (Operation) {
        case "addition":
            System.out.println(num1 + num2);
            break;
        case "subtraction":
            System.out.println(num1 - num2);
            break;
        case "multiplication":
            System.out.println(num1 * num2);
            break;
        case "division":
            if (num1 == 0 || num2 == 0) ;
        {
            System.out.println("You can not divide by or with a zero!");
        }

        System.out.println(num1 / num2);
        break;
        default:
            System.out.println("Invalid Operation!");
    }

   }
}

Style

Your structure for this program is fine, except one bug highlighted below. However, from first glance, you can make some style adjustments to increase the readability of your program. Coding conventions are extremely important - and although things like "do I put brackets on the same line or  a new line?" are controversial, it is essential that you write clean and readable code. A little spacing never hurt anybody, but it should be consistent and clean.

float num2= input.nextFloat();

Put a space before the equal sign.

String Operation = input.next();

Variable names should typically be lower camel case. Operation is actually a class from the java.rmi.server library, so you should rename this to simply operation.

case "division":
    if (num1 == 0 || num2 == 0) ;
{
    System.out.println("You can not divide by or with a zero!");
}

There should not be a semi-colon after the if-statement. This is causing wrong output! Line up your brackets. Also, why can't you divide 0? 0 / 5 is a perfectly valid operation.

You can clean this up simply by using the following

case "division":
    if (num2 == 0)
        System.out.println("You cannot divide by zero!");
    else
        System.out.println(num1 / num2);
    break;

You could also keep the brackets if you wish. If the statement contains only one inner statement, you don't need brackets, but some people prefer it for the readability. This is a personal style.

    case "division":
        if (num2 == 0) {
            System.out.println("You cannot divide by zero!");
        } else {
            System.out.println(num1 / num2);
        }
        break;

If you really want to trim it down, you can use the ternary operator ?.

    case "division":
        System.out.println(num2 == 0 ? "You cannot divide by zero!" : (num1 / num2));
        break;

Once you've done that, challenge yourself by getting yourself familiar with reading input files, so you don't have to type your input every time! You could try experimenting with a BufferedReader for example.


Checking for valid date range in Rails I am getting a date range from form inputs - the "from" month, day, and year, and the "to" month, day, and year, and then running a report on data for within that date range. I want to first check that the dates are valid and if not display an error indicating dates are not valid, then run the report only if valid. I want to

make sure the dates themselves are valid, i.e., no blank inputs, and nothing like February 30th, then
make sure the "from" date is before the "to" date.

I am having trouble doing this in a clean way with repeating myself a bunch. Here's the long way. Please help refactor with assigning to variables but only if valid.

unless Date.valid_date?(params[:from][:year].to_i,
                        params[:from][:month].to_i,
                        params[:from][:day].to_i) &&
  Date.valid_date?(params[:to][:year].to_i,
                   params[:to][:month].to_i,
                   params[:to][:day].to_i)
  redirect_to :back, alert: "Please provide valid date values." and return
end
unless Date.new(params[:from][:year].to_i,
                params[:from][:month].to_i,
                params[:from][:day].to_i) <
  Date.new(params[:to][:year].to_i,
           params[:to][:month].to_i,
           params[:to][:day].to_i)
  redirect_to :back, alert: "Please provide valid date values." and return
end
from_date = Date.new(params[:from][:year].to_i,
                     params[:from][:month].to_i,
                     params[:from][:day].to_i)
to_date = Date.new(params[:to][:year].to_i,
                   params[:to][:month].to_i,
                   params[:to][:day].to_i)
run_report(from_date, to_date)

DRY - don't repeat yourself - basically means that you should extract repeated logic into methods.

In this case you're repeating

checking validity
creating Date instances (if they're valid, you should just create them, and then compare them; it's madness to create them for the comparison only to re-create them a few lines later)
... and for each of those, you're extracting 3 specific values from a params hash

So you could for instance do this instead:

# Returns a Date, or nil if the input isn't valid
def params_to_date(params = {})
  args = %w(year month day).map { |k| params[k] }
  Date.new(*args) if Date.valid_date?(*args)
end

# ...

def some_controller_action
  from_date = params_to_date(params[:from])
  to_date = params_to_date(params[:to])

  if from_date && to_date && from_date < to_date
    run_report(from_date, to_date)
  else
    redirect_to :back, alert: "Please provide valid date values."
  end
end


Given an element, returning the inorder rank of it an in a binary search tree with only two parameters I have a solution here, but I'm not too fond of it because I feel like it can be done better.

template <typename Comparable>
bool AugmentedBinarySearchTree<Comparable>::Rank(const Comparable& x, BinaryNode* t, int* nodesVisited) const {
  if (t == NULL) {
    return false;
  }
  if (Rank(x, t->m_left, nodesVisited)) {
    return true;
  }
  *nodesVisited += 1;
  if (t->m_element == x) {
    return true;
  }
  return Rank(x, t->m_right, nodesVisited);
}

My biggest stumbling block was finding way to both confirm the node actually existed and also returning its rank.

I started with this method signature

int BinarySearchTree<Comparable>::Rank(const Comparable& x, BinaryNode* t) const {

but in the end I had to make the method return a bool and add an extra parameter to keep track to where the program was in the traversal.

Is there any way to do this without the nodesVisited parameter?

I'd recommend the following approach. Split up Rank into a public-facing member function and a private-facing member function. The private one's signature is:

template <typename Value>
std::pair<bool, int> RankImpl(BinaryNode* tree, const Value& value);

The public one is:

template <typename Value>
boost::optional<int> Rank(BinaryNode* tree, const Value& value);

(Secondarily, I flipped the order of your arguments, provided better names for them, and am not limiting the value we're looking for to be a Comparable. Rather, anything that is equality-comparable to Comparable should suffice).

The idea of using boost::optional is that it, as a return type, already encompasses what you want in a single type - it tells you if there is or isn't a value (i.e. was value found or not) and if there is a value, what it is (i.e. its rank). We however privately still need a value even in the false case, hence the pair helper function.

Altogether:

template <typename Value>
boost::optional<int> Rank(BinaryNode* tree, const Value& value)
{
    auto pr = RankImpl(tree, value);
    if (pr.first) {
        return pr.second;
    }
    else {
        return boost::none;
    }
}

template <typename Value>
std::pair<bool, int> RankImpl(BinaryNode* tree, const Value& value)
{
    if (!tree) {
        return {false, 0};
    }

    auto left = RankImpl(tree->left, value);
    if (left.first) {
        return left;
    }
    else if (tree->m_element == value) {
        return {true, left.second + 1};
    }
    else {
        auto right = RankImpl(tree->right, value);
        return {right.first, left.second + 1 + right.second};
    }
}


CRUD integration test, is it ok? I want to test my CRUD operation to a MySql database (I use dapper as my ORM).
This is what I wrote with xUnit:
    private IPaymentRepository paymentRepository;
    private Payment testPayment;
    private string connectionString = "Database=..."; //Test DB

    public PaymentRepositoryTest()
    {
        paymentRepository = new PaymentRepository(connectionString);
        //My model has private setters so I have a builder to initialize the object.
        testPayment = new Payment.Builder()
            .Id(0)
            .Description("Test")
            .Type(1)
            .WithPercentageDiscount(10)
            .WithAdvancePercentageDiscount(0)
            .WithInstallments(new int[] { 30 }).Build;
    }

CREATE
[Fact]
    public void Insert_Payment_ReturnId()
    {
        long id = 0;
        using(var transaction = new TransactionScope())
        {
            id = paymentRepository.Insert(testPayment);
        }

        Assert.True(id > 0);
    }

READ
[Fact]
    public void Get_Payment_ReturnPaymentFromDb()
    {
        using (var transaction = new TransactionScope())
        {
            long id = paymentRepository.Insert(testPayment);
            var payment = paymentRepository.GetById(id);

            Assert.NotNull(payment);
        }
    }

UPDATE
I don't really like this code, having private setter here is unfortunate because I have to recreate the object. I'm also not really sure about checking only one field.
    public void Update_Payment()
    {
        long id = 0;

        const string UPDATED_DESCRIPTION = "Test2";

        using (var transaction = new TransactionScope())
        {
            id = paymentRepository.Insert(testPayment);

            var updatedPayment = new Payment.Builder()
            .Id(id)
            .Description(UPDATED_DESCRIPTION)
            .Type(1)
            .WithPercentageDiscount(10)
            .WithAdvancePercentageDiscount(0)
            .WithInstallments(new int[] { 30 }).Build();

            paymentRepository.Update(updatedPayment);

            updatedPayment = paymentRepository.GetById(id);

            Assert.Equal(UPDATED_DESCRIPTION, updatedPayment.Description);
        }
    }

DELETE
Just like the update, I don't like having to recreate the object just to have the proper ID.
[Fact]
    public void Delete_Payment_ReturnNull()
    {
        using (var transaction = new TransactionScope())
        {
            long id = paymentRepository.Insert(testPayment);

            var paymentToDelete = new Payment.Builder()
            .Id((int)id)
            .Description("Test")
            .Type(1)
            .WithPercentageDiscount(10)
            .WithAdvancePercentageDiscount(0)
            .WithInstallments(new int[] { 30 }).Build();

            paymentRepository.Delete(paymentToDelete);

            var payment = paymentRepository.GetById(id);

            Assert.Null(payment);
        }
    }

The code of course works and I get the proper results, but I feel I'm repeating basically the same test over and over especially in Update and Delete.
Is this integration code good ? How can I make it better ?

Adding automated integration tests is a great way to take testing to the next level, and what you've written is really good for a first attempt. Here are some pointers/suggestions that might help you along your way.

It's almost inevitable that your tests will end up effectively testing the same thing multiple times. As you've identified, most of your tests use the GetById method, and in fact all of them Insert a payment. The only potential problem this causes is that if one of those methods stops working you suddenly have most/all of your tests failing and it may not be immediately obvious where the problem is. In the grand scheme of things, this isn't that much of an issue as you'll almost certainly be able to figure out where the problem is by looking at the test output and/or by stepping through the test with a debugger.

Although it seems like your tests are very similar, a lot of the similarities are down to them sharing common setup steps, e.g. inserting a payment so you have something to get/update/delete. It is absolutely fine to have this sort of duplication in unit/integration tests, and again is inevitable in most circumstances. As the number of tests grows, you might find that it makes sense to pull out some of the setup into a separate method that's used by multiple tests. This is also fine to do, as long as it doesn't affect the readability of the test, so avoid using generic method names like Setup() and instead prefer clear (if slightly verbose) names like CreateRepositoryWithSinglePayment(testPayment).

The fluent payment builder makes it hard to know which properties are required and which are optional (if any). I can't advise a great deal on this without seeing the Payment or PaymentBuilder classes, but if all of the properties are required, consider just creating a Payment constructor that assigns parameter values to readonly properties. Then there's no doubt about what values are required to create a valid Payment.

One thing to be aware of when integration testing is that it's often easy to end up with shared state between tests. Here, you are inserting a payment into your database as part of every test and this is not always deleted afterwards (or at least there's no tear-down code included in your post). Since you are actually persisting something in a database, it will therefore exist across tests unless explicitly deleted after each test is run. This could result in some very confusing test behaviour in the future as your tests might not be testing what you intended! You should add a tear-down step by implementing IDisposable and deleting every payment in your test database inside the Dispose() method. This ensures that every test will start from the same fresh state.
On a similar note, be aware that you can get strange behaviour when running integration tests in parallel if they share the same data store(s). You're fine at the moment because by default xUnit runs test cases within the same class in serial, but if you had another test class using the same database these would be run in parallel by default. There are ways to control this behaviour,  but it's more something to be aware of as this shouldn't be an issue given your current test organisation.

You shouldn't need to provide the entire Payment object to delete it from the database. If you can read a payment by its Id using the GetById method then you should also be able to delete a payment by its Id. This should simplify the Delete test case somewhat.

There are plenty of other things you could test here. You've got tests for the 'happy' paths, i.e. the paths where everything goes as expected, but there are certainly lots of edge cases (or unexpected cases) that you could also test. Maybe you've already considered some/all of these, but off the top of my head:

What happens when you insert a payment that already exists in the database?
What happens when you try to read a payment that does not exist in the database?
What happens when trying to update a payment that doesn't exist in the database?
Can you update the payment with a null description? What happens if the description is really long or contains special characters? You'd probably want to use a [Theory] with [InlineData] to enumerate several different test cases if doing this.
What happens when updating properties other than the description?
What happens when trying to delete a payment that doesn't exist in the database?


Simple activation script The script is fully working, but I ask is there a better way I can do this rather than name all of the query variables, $q, $q1 etc, and then use if else statements to execute the code.

<?php
// open the mysql connection

require_once('mysqli.php');

// successful login variables

$q = $dbc -> prepare("SELECT * FROM accounts WHERE email = ? && password = ? && logcount != ''");
$q -> bind_param('ss', ($_POST['email']), ($_POST['password']));
$q -> execute();
$q -> store_result();

// wrong password variables

$q1 = $dbc -> prepare("SELECT * FROM accounts WHERE email = ? && password != ? && logcount != ''");
$q1 -> bind_param('ss', ($_POST['email']), ($_POST['password']));
$q1 -> execute();
$q1 -> store_result();

// not active variables

$q2 = $dbc -> prepare("SELECT * FROM accounts WHERE email = ? && logcount = ''");
$q2 -> bind_param('s', ($_POST['email']));
$q2 -> execute();
$q2 -> store_result();

// successful login query

if ($q -> num_rows == 1) {
    $q = $dbc -> prepare("UPDATE accounts SET logcount = logcount+1 WHERE email = ?");
    $q -> bind_param('s', ($_POST['email']));
    $q -> execute();
    $dbc -> close();
    header('location: shack');
    exit();
}

// wrong password query

elseif ($q1 -> num_rows == 1) {
    echo '
Your password is incorrect.
';
}

// not activated query

elseif ($q2 -> num_rows == 1) {
    echo '
Your account is not activated.
';
}

// blank email form entry

elseif (empty($_POST['email'])) {
    echo '
You did not fill in an email on the login form.
';
}

// account not found

else {
    echo '
An account could not be found.
';
}

// close the mysql connection

$dbc -> close();

?>

Appears you are storing cleartext passwords and doing a 1:1 comparison.  This is bad.

You can clean the code up by doing the following and executing 1 query to MySQL:

require_once('mysqli.php');
// successful login variables
$q = $dbc -> prepare("SELECT email,dbClearTextPassword,accountActive FROM accounts WHERE email = ?");
$q -> bind_param('s', ($_POST['email']));
$q -> execute();
$q -> store_result();

// check to see if you returned any information from your query
if ($q->num_rows == 0) {
  // email doesn't exist in db .. #fail
  exit;
}

if ($q['accountActive'] != '1') {
  // email account is not active .. #fail
  exit;
}

// check the password matches
if ($_POST['clearTextPassword'] == $q['dbClearTextPassword']) {
  // do some of your fancy code to process the successful login
} else {
  // do some other fancy code to process a login failure
}

$dbc->close();


Simple quicksort implementation in C++ #include <iostream>

using std::cout;
using std::endl;

int arr[] = {3,4,2,1,-5,116,31,4,0};

void QuickSort(int start,int end);
int Partition(int start, int end);
void Swap(int * , int *);
void PrintArr();

int main(int argc, char* argv[])
{

    PrintArr();

    QuickSort(0, sizeof(arr)/sizeof(int));

    PrintArr();

    return 0;
}

void QuickSort(int start,int end)
{
    if(start >= end) return;

    int index = Partition(start , end);

    QuickSort(start, index - 1);
    QuickSort(index + 1, end);
}

int Partition(int start, int end)
{
    int index = start;
    int pivot = arr[end-1];
    for(int i = start;i < end - 1;i++)
    {
        if(arr[i] <= pivot)
        {
            Swap(arr + index,arr + i);
            index++;
        }
    }

    Swap(arr + index, arr + end - 1);

    return index;

}

void Swap(int *a , int *b)
{
    if(*a != *b)
    {
        int temp = *a;
        *a = *b;
        *b = temp;
    }
}

void PrintArr()
{
    for(int i = 0;i<sizeof(arr)/sizeof(arr[0]); i++)
        cout<<arr[i] <<" ";

    cout<<endl;
}

Writing an implementation of quicksort is fairly easy. Writing a good implementation that works at least reasonably well with almost any input is much more difficult.

Your uses a choice of pivot element that's pretty well know for bad behavior. By choosing the last element in a range (or the first) you essentially guarantee that it runs in \$O(N^2)\$ if the input is sorted. One typical way of preventing this problem is to use a "median of three" to select the pivot element--that is, look at the first, middle and last elements, and use the median of those three as the pivot.

There's one more minor detail there though: you don't just want to choose the median of those three and use it as your pivot. If those three elements are out of order (with respect to each other) you want to sort them, and write them back to the array in their sorted order. This improves behavior (somewhat) during the recursive calls.

Another subtlety that important to correct behavior (rather than just performance) is to use the correct order for your recursive calls. To assure against stack overflow, you always want to sort the smaller of the two partitions first. In conjunction with tail call elimination (which compilers have practiced for years) this can eliminate the possibility of stack overflow. If you want to be even more certain about it, you can change the final recursive call to iteration, so it's not up to the compiler to eliminate the tail recursion.

C++ adds another interesting twist, at least as soon as you start to deal with sorting collections of arbitrary types of objects (rather than just int). In this case, a user may well have written their own swap function that they expect you to use to swap objects of a particular type. Otherwise, you normally want to use std::swap rather than writing a swap of your own. To select between those correctly, you normally want to put using std::swap at the beginning of your partition function, and then use unqualified calls to swap (e.g., do not explicitly call std::swap). This way if the user has written a swap in the same namespace as the type you're sorting (and named it swap, like they should) your sorting routine will find that via ADL, and use it. Otherwise, it'll find std::swap because of the using std::swap;, and use that. This gives the behavior we want: use their swap routine by preference, but use std::swap if they haven't provided one.

A few other points: of course, the standard library has std::sort built in, which is typically implemented as introsort (which is just a quicksort that keeps track of recursion depth, and switches to a heap sort if quicksort is doing poorly for the input it's been given).

This code doesn't really look/feel much like most people expect C++ to be written. Right now, you're mostly using (and passing) indexes into the array. In C you'd more likely use pointers. In C++, you'd probably want to use iterators instead (though you can pass pointers as iterators). In C++ you'd also normally want something like this to be template, so it can be applied to almost any type of object, not just int. In addition, you'd typically allow the user to pass a comparison function so (for example) if they want to sort Person objects that don't support comparison using <, the user can still sort by passing (for example) a lambda expression saying they want to sort by a person's last name and first name (and default to std::less<T>, so if < is defined, it can be used by default, though the user can still pass a different comparison if they want).


Party Invitation - Trimming the Invitation List I am learning Java right now and am coding some simple programs to practice. Right now, one of my for loops has an inelegant way of ending the loop. I was wondering if there was another way to express what I want the program to do.

Problem: Party Invitation

  Problem Description

    Number your friends 1,2,....,K and place them in a list in this order. Then perform m rounds. In each round, use a number to determine which friends to remove from the ordered list.

    The numbers will use numbers r1, r2,....,rm. In round 'i' remove all the remaining people in positions that are multiples of ri (that is, ri, 2ri, 3ri,...) The beginning of the list is position 1.

    Output the numbers of the friends that remain after this removal process.

  Input Specification

    The first line of input contains the integer K. The second line of input contains the integer m, which is the number of rounds of removal. The next m lines each contain one integer. The 'i'th of these lines contains ri indicating that every person at a position which is multiple of ri should be removed.

  Output Specification

    The output is the integers assigned to friends who were not removed. One integer is printed per line in increasing sorted order.

  Sample Input

    10

    2

    2

    3

  Output for Sample Input

    1

    3

    7

    9

  Explanation of Output for Sample Input

    Initially our list of invitees in 1,2,3,4,5,6,7,8,9,10. There will be two rounds of removals. After the first round of removals, we remove the even positions (because round 1 is '2', so every second position), which causes our list of invitees to be 1, 3, 5, 7, 9. After the second round of removals. we move every 3rd remaining invitee: thus, we keep 1 and 3, remove 5 and keep 7 and 9, which leaves us with an invitee list of 1, 3, 7, 9.

My code has a few System.out.printlns so I can keep track of what it is doing.

public class Party {

public static void main(String args[]) {
    try {

        BufferedReader input = new BufferedReader(new InputStreamReader(
                System.in));

        int totalFriends = Integer.parseInt(input.readLine());
        int rounds = Integer.parseInt(input.readLine());
        int[] roundNumber = new int[rounds];
        ArrayList<Integer> friendList = new ArrayList<Integer>(totalFriends);

        // create my list of friends
        for (int i = 0; i < totalFriends; i++) {
            friendList.add(i + 1);
        }

        // create an array for the number of rounds to cut, and how to cut
        // each time
        for (int i = 0; i < rounds; i++) {
            roundNumber[i] = Integer.parseInt(input.readLine());
        }

        System.out.println("Rounds: " + Arrays.toString(roundNumber));

        // for each round, do this
        for (int h = 0; h < rounds; h++) {

            System.out.println("Removing every " + roundNumber[h]
                    + " number");

            System.out.println("Starting with: " + friendList);

            int indexToRemove = 0;

            //the for loop here tells it to exit when the indexToRemove is larger
            //than the friendlist. I have to say +roundNumber[h] - 1, because the indexToRemove is
            //from the last iteration, not the one I want it to check.

            for (int i = 0; (indexToRemove + (roundNumber[h] - 1)) < friendList
                    .size(); i++) {

                indexToRemove = ((roundNumber[h] - 1) + (i * (roundNumber[h] - 1)));
                System.out.println("Removing index: " + indexToRemove);

                friendList.remove(indexToRemove);

                System.out.println(friendList);

            }

        }

        // the required output for this problem
        for (int i = 0; i < friendList.size(); i++) {
            System.out.println(friendList.get(i));
        }

    } catch (IOException e) {
        e.printStackTrace();
    }
}
}

for loop

You're right.  Not much point in checking the boundary condition twice.

        // right now, it is doing 1 more calculation than necessary
        // before exiting the for loop
        int indexToRemove = 0;

        for (int i = 0; indexToRemove < friendList.size(); i++) {
            // what I'm trying to say is that if the index you're
            // cutting is longer than the friendList, then exit the for
            // loop
            // but the indexToRemove here is from the last iteration, so
            // it doesn't work
            indexToRemove = ((roundNumber[h] - 1) + (i * (roundNumber[h] - 1)));
            System.out.println("Removing index: " + indexToRemove);
            if (indexToRemove < friendList.size()) {
                friendList.remove(indexToRemove);
            }
            System.out.println(friendList);

        }

You need to update indexToRemove at the end of the loop, not the beginning.

        int indexToRemove = roundNumber[h] - 1;

        for (int i = 1; indexToRemove < friendList.size(); i++) {
            System.out.println("Removing index: " + indexToRemove);
            friendList.remove(indexToRemove);
            System.out.println(friendList);
            indexToRemove = ((roundNumber[h] - 1) + (i * (roundNumber[h] - 1)));
        }

Perhaps this would be clearer as a while loop:

        int indexToRemove = roundNumber[h] - 1;
        int i = 1;

        while ( indexToRemove < friendList.size() ) {
            System.out.println("Removing index: " + indexToRemove);
            friendList.remove(indexToRemove);
            System.out.println(friendList);

            indexToRemove = ((roundNumber[h] - 1) + (i * (roundNumber[h] - 1)));
            i++;
        }

Remember that you can always write a for loop as a while loop.  It's just syntactic sugar.  Or vice versa:

        for (int indexToRemove = roundNumber[h] - 1, i = 1; indexToRemove < friendList.size(); i++, indexToRemove = ((roundNumber[h] - 1) + (i * (roundNumber[h] - 1))) ) {
            System.out.println("Removing index: " + indexToRemove);
            friendList.remove(indexToRemove);
            System.out.println(friendList);
        }

Although that's a bit long and complicated for a single line.  Note that we can rewrite the first line as

        for (int indexToRemove = roundNumber[h] - 1, i = 1; indexToRemove < friendList.size(); i++, indexToRemove = (i + 1) * (roundNumber[h] - 1) ) {

That's a bit better, but we can still do better:

        for (int indexToRemove = roundNumber[h] - 1; indexToRemove < friendList.size(); indexToRemove += roundNumber[h] - 1 ) {

That seems more like a for loop.  We've done away with i altogether and only work with indexToRemove.


Is it a good idea to have a public inner class as a runner for a service? I have a service class that handles recipe management using a piquetteClient (piquette is a web app with a rest API that can run puppet scripts).

The thing is this service class has a bunch of methods like runRecipe, searchRecipes, filter, list etc, and they all require a piquetteClient instance. Because I wanted to not be able to run any of this methods without having first a valid instance of piquetteClient and because I don't want to have it as a parameter for each method (and don't also I don't like to have it externally with a setter because then you give the user the option to just not call that setter - it's confusing), I create a public inner class Runner inside this service and buildRunner method outside the inner class that can give you an instance of the runner only after you provide a valid piquette client instance, after that, the same instance will be used everywhere.

What do you think?

@Component
public class RecipeServiceImpl implements RecipeService {

    private static final long serialVersionUID = 1000050450505050443L;

    @Override
    public Runner buildInternalRunner(PiquetteClient piquetteClient) {
        // validate client instance
        Runner runner = new Runner(piquetteClient);
        return runner;
    }

    public class Runner {

        private final PiquetteClient piquetteClient;

        Runner(PiquetteClient piquetteClient) {
            this.piquetteClient = piquetteClient;
        }

        public List<Recipe> searchRecipes(String query, int limit, int offet) {
            return filterRecipes(piquetteClient.recipes(query, Integer.MAX_VALUE, 0), query, limit);
        }

        /**
         *
         * @param recipeName
         * @param asset
         * @return Future of Integers
         */
        @Async
        public Future<Integer> runRecipe(String recipeName, String asset) {
            // trying to get the correct asset
            String regexAsset = RegexUtil.ANY_CHAR + asset + RegexUtil.ANY_CHAR;
            List<Artifact> artifacts = listArtifacts(recipeName, regexAsset);
            if (!artifacts.isEmpty()) {
                if (artifacts.size() > 1) {
                    log.warn("Multiple artifacts matched... choosing the first one: " + artifacts.get(0).getBuild());
                }
                URI uri = artifacts.get(0).getBuild();
                log.info("Running: " + artifacts.get(0).getBuild());
                return new AsyncResult<>(piquetteClient.runRecipe(recipeName, uri).getId());
            } else {
                log.error("Nothing for: " + recipeName + " and asset: " + regexAsset);
            }
            return new AsyncResult<>(NumberUtils.INTEGER_ZERO);
        }

        // other similar methods
}

RecipeService interface:

public interface RecipeService extends Serializable {
     RecipeServiceImpl.Runner buildInternalRunner(PiquetteClient piquetteClient);
}

I can't use the constructor because RecipeServiceImpl is a Spring component. The build method it's an override because it's declared in the Service interface so I can inject the service everywhere and use the method. Maybe the name Runner is a bad one, but I was asking how this concept feels like; you have a service implementation that has another object inside that does the actual job. The service itself is just validating and delegating... From outside you can't have a reference to the runner object without having first an instance of the service itself and provide to this instance the necessary data.

I'm a bit confused by your code -- RecipeServiceImpl.buildInternalRunner has a return type of RecipeServiceImpl.Runner, and it's an Override?  Really?

Runner seems to be badly named, as it is easily confused with Runnable, which means something very different.  Is this really a RecipeServiceClient?

It's additionally confusing because you seem to be suggesting that RecipeService declares searchRecipes, but your runner doesn't implement that interface....

Given what you have shown thus far, I would have expected to see a single class, call it PiquetteRecipeService, that extends RecipeService but otherwise looks like the Runner you implemented.

class PiquetteRecipeService implements RecipeService {
    private final PiquetteClient piquetteClient;

    PiquetteRecipeService(PiquetteClient piquetteClient) {
        this.piquetteClient = piquetteClient;
    }

    public List<Recipe> searchRecipes(String query, int limit, int offset) {
        ...
    }

    // And so on....
}

This implementation does require that a PiquetteClient be created before the Service is created, but if the service can't do anything without the client, then that's the Right Thing[tm].

If that's not the right answer, then I think you haven't provided a clear description of the constraints you are trying to satisfy.

public interface RecipeService extends Serializable {
    RecipeServiceImpl.Runner buildInternalRunner(PiquetteClient piquetteClient);
}

OK, this is beyond broken.  You've got an interface, that declares a function which returns a type defined by a specific implementation of that interface.  Help, help, I'm being recursed!

BUT...

The good news is that you are clearly groping toward a Factory pattern.  If we change the spelling of a few things...

public interface RecipeServiceFactory {
    RecipeService createService(PiquetteClient piquetteClient);
}

In other words, instead of expecting Spring to create the service for you, you tell it to create an instance of the service factory for you, and then use that factory to create instances of the service when necessary.

public class DefaultRecipeServiceFactory {
    RecipeService createService(PiquetteClient piquetteClient) {
        return new PiquetteRecipeService(piquetteClient);
    }
}

Meanwhile, you would have a new interface declaration, using the old name...

public interface RecipeService {
    List<Recipe> searchRecipes(String query, int limit, int offet);
    Future<Integer> runRecipe(String recipeName, String asset);
    // ...
}

and the PiquetteRecipeService would implement this interface, delegating calls to the client.

Going back to your original question; with this design, yes it does it make sense to combine the service implementation and the factory that is coupled to it in a single class file.  I'd be more likely to nest the factory inside the service implementation, rather than the other way around:

public class PiquetteRecipeService implements RecipeService {
    public static class Factory implements RecipeServiceFactory {
        // ...
    }
}

If the service implementation isn't supposed to be public, then I would nest them the other direction (or keep them completely separate).

I still wouldn't quite be happy with this design, because arranging for the Factory to accept a PiquetteClient argument rather gives away the fact that the you are using Piquette under the covers.

Spring basically is a giant factory, so why do we need to roll our own?  If you were to let Spring create the PiquetteClient, for instance, then it could inject that client into the RecipeService it creates too.  If the problem there is that you need credentials, or a configuration or something to create the PiquetteClient, then maybe that's the argument that should be passed to the Factory.  Unclear -- just noting that as it stands, it is a bit odd.


How to store option key names I have to store some properties in a database properties table which looks like this:

CREATE TABLE `property` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `computer_name` varchar(64) NOT NULL,
  `name` varchar(64) NOT NULL,
  `value` varchar(1024) NOT NULL,
  PRIMARY KEY (`id`)
);

Sample data:

+----+---------------+-----------------+-------+
| id | computer_name | name            | value |
+----+---------------+-----------------+-------+
|  9 | PC002         | firewall_status | 3     |
| 10 | PC011         | firewall_status | 0     |
| 11 | PC011         | some_property   | 1     |

To access data in that table I have a getProperty($computerName, $propertyName) method in a repository, which finds the value of specified property for certain computer.
And in different parts of my application I call the method like this:

$computer = $repository->findComputer();
$firewallStatus = $repository->getProperty($computer->name, 'firewall_status');

What I don't like here is hard coded name of the property.
My solution was to put available property names to a model:

class ComputerModel
{
    const PROPERTY_FIREWALL_STATUS = 'firewall_status';
    const PROPERTY_SOME_PROPERTY   = 'some_property';
}

...

$computer = $repository->findComputer();
$firewallStatus = $repository->getProperty($computer->name, ComputerModel::PROPERTY_FIREWALL_STATUS);

We discussed storing property names as constants and decided that we have some trade-offs:

Good (mostly for code maintanance):

keep the value in one place which make it more simple to change it;
keep available property dictionary in one place (in the model);
make the code completion available in IDE instead of searching for correct property name to copy/paste it;

Not good (mostly for debugging):

by storing a literal values we create extra overhead by storing
variable names in new variables;
worsen debugging by add one more step to find the object containing debug information;
we need to support additional dictionary; possible data inconsistency if someone adds new value to database but forget to update it in the model.

Subjective:

improves/reduces code readability.

The question is: what is the best way to store such parameter names? Or maybe to use literals.

keep the value in one place which make it more simple to change it

You will never change it (why would you?) so this is not a valid reason. And if you did change it then search/replace works fine.

  keep available property dictionary in one place (in the model)

This looks like MySQL to me. So I suggest using an enum for this instead of a varchar. It will be much faster, take less storage space, prevent invalid values, and have the benefit of storing the dictionary right in the database.

  make the code completion available in IDE instead of searching for correct property name to copy/paste it

This  is nice - however is there no other way to do this? Can you not add some kind of configuration file that will define these?

In the "not good" column I would put adds complexity for little gain. You've taken a string, and turned it back into a different string, only uppercase.

Now, if you were going to store these as integers (i.e. do the equivalent of enum yourself) then making constants has value. You are turning a string into a number, so it actually does something.

So, my suggestion:

Either use enums in your database, and just write them as strings, or store integers, and use the constants.

But don't store these properties as strings.

(BTW, suppose you want to convert from one to the other: Search/replace! I've seen too many cases of overengineering things just from fear of search/replace.)


Subclassing UIButton I need to subclass UIButton, however the design of UIButton makes this really painful. This is because to create a button you really need to call:

[UIButton buttonWithType:UIButtonTypeRoundedRect]

So if I want to subclass I really need to do whatever it is this method does. However, it is not possible to set the buttons underlying type.

One option is to use something like decorator and subclass UIButton but have the actual button as an ivar and defer all property/method calls to this button. The problem here is that I then need to implement most of the UIButton interface (yawn).

What I have done is something a bit sneakier in my sub class. I have created this method:

- (id) initWithType:(UIButtonType)buttonType
{
    [super init];

    actualButton = [[UIButton buttonWithType:buttonType] retain];

    memcpy(self, actualButton, sizeof(UIButton));
    self->isa = [ABDescriptorButton class];

    return self ;
}

I know this is pretty unorthodox, but it works and is fairly clean in terms of code. What are the pitfalls I might have with this approach?

You don't need to make an initWithType: method. Just override the buttonWithType: factory method.


Slot Machine Simulator I'm studying alone, so I don't know if I'm writing efficiently. Could this slot machine simulator be improved? How?

import acm.program.*;
import acm.util.*;

/*
 * THIS PROGRAM SIMULATES THE SLOT MACHINE
 */

public class cap6ex5 extends ConsoleProgram{

private RandomGenerator rgen = RandomGenerator.getInstance();

private int money = 50; //starting stake

private String value = "";

public void run(){

    String instAnswer = readLine("Would you like instructions?");

    while (!instAnswer.equals("yes") && !instAnswer.equals("no")) {
        instAnswer = readLine("Answer yes or no.");
    }
    if (instAnswer.equals("yes")) println("YOU SHOUD KNOW!");

    startGame(money);

}
private void startGame(int x){
    String keepPlaying = "";
    int finalMoney = x;
    int prize = 0;
    while (finalMoney > 0){
        keepPlaying = readLine("You have $" + finalMoney +". Would you like to play?");
        //avoid different answers
        while(true){
            if (keepPlaying.equals("yes") || keepPlaying.equals("no")) break;
            keepPlaying = readLine("Asnwer yes or no.");

        }
        if (keepPlaying.equals("no")) break;

        finalMoney--;
        prize = gamePrize();
        finalMoney += prize;
        println(value + "     -- You win $" + prize);
    }

    println("Okay, bye. You ended with $" + finalMoney);
}
private int gamePrize(){
    int countBAR = 0;   //BAR == 1
    int countBELL = 0;  //BELL == 2
    int countPLUM = 0;  //PLUM == 3
    int countORANGE = 0;//ORANGE == 4
    int countCHERRY = 0;//CHERRY == 5
    value = "";

    for (int i = 1; i <= 3 ; i++){
        int x = rgen.nextInt(1, 6);

        switch(x){
            case 1:
                countBAR++;
                value += "BAR ";
                break;
            case 2:
                countBELL++;
                value += "BELL ";
                break;
            case 3:
                countPLUM++;
                value += "PLUM ";
                break;
            case 4:
                countORANGE++;
                value += "ORANGE ";
                break;
            case 5:
                countCHERRY++;
                value += "CHERRY ";
                break;
            case 6: value += "LEMON "; break;
        }

    }

    return result(countBAR, countBELL, countPLUM, countORANGE, countCHERRY);

}
private int result(int bar, int bell, int plum, int orange, int cherry){
    int prize = 0;
    if (bar == 3){
        prize = 250;
    } else if ((bell > 1 && bar ==1) || bell == 3){
        prize = 20;
    } else if ((plum > 1 && bar == 1) || plum == 3){
        prize = 14;
    } else if ((orange >1 && bar == 1) || orange == 3){
        prize = 10;
    } else switch(cherry){
        case 1: prize = 7; break;
        case 2: prize = 5; break;
        case 3: prize = 2; break;
    }
    return prize;
}

}

Here are the prize rules:

  BAR BAR BAR = $250
  BELL BELL BELLorBAR = $20
  PLUM PLUM PLUMorBAR = $14
  ORANGE ORANGE ORANGEorBAR = $10
  CHERRY CHERRY CHERRY = $7
  CHERRY CHERRY --- = $5
  CHERRY --- --- = $2

When comparing against "yes" or "no", you should consider using equalsIgnoreCase so that answers like YES can be accepted. (Perhaps the player is excited to play?)
startGame: I cannot tell what the x argument is.  It should be renamed.
gamePrize: Rather than hard-coding the 1-6 figures in your switch statement, you should make each one a constant (ex. VAL_ORANGE).
result: I would not know that you arguments are the number of occurrences of each type.  These should probably be renamed.
result: You currently have 1 cherry as winning $7 and 3 as winning $2.  This appears to be a bug.


Inserting and updating time spans in XML I need help to refactor my code. I usually had a hard time figuring out how to make my code reusable.

I have an XML file that hold the data for each Tag element. Tag element should have child nodes LastClocked, and TotalClocked. I first thought of creating Tag object and do serialization. But, I found Linq to XML is much easier. I would really appreciate if you guys can tell me what to improve for my code.

namespace StopWatch.Models
{
    public class TagCollection
    {
        private XElement doc;
        private IEnumerable<XElement> tagElements;

        public TagCollection()
        {
            if(File.Exists("TagsData.xml"))
            {
               doc = XElement.Load("TagsData.xml");
            }
            else
            {
                //TODO: Create XML
            }
        }

        public void Save(TimeSpan clocked, string tags)
        {
            tagElements = from t in doc.Elements("Tag")
                          where (string)t.Attribute("Name") == tags
                          select t;

            TimeSpan lastClocked = TimeSpan.Parse((string)
                                   (from lc in tagElements.Descendants("LastClocked")
                                    select lc).First());

            lastClocked = lastClocked.Add(clocked);

            if (!tagElements.Any())
            {
                Insert(clocked, tags);
            }
            else
            {
                Update(clocked, lastClocked);
            }

            doc.Save("TagsData.xml");
        }

        private void Update(TimeSpan clocked, TimeSpan lastClocked)
        {
            foreach(XElement tagElement in tagElements)
            {
                tagElement.SetElementValue("LastClocked", clocked.ToString());
                tagElement.SetElementValue("TotalClocked", lastClocked.ToString());
            }
        }

        private void Insert(TimeSpan clocked, string tags)
        {
            XElement newTag = new XElement("Tag",
                                    new XAttribute("Name", tags),
                                    new XElement("LastClocked", clocked.ToString()),
                                    new XElement("TotalClocked", clocked.ToString()));

            doc.Add(newTag);
        }
    }
}

Your code seems very procedural - you're worrying about implementing how things will be done before you think about what those things are and how you'd like to use them.  You don't have any properties, just methods, which I don't particularly like.

From what I understand you want to write a reusable wrapper from some XML, that's going to fit in a consistent format.  Do you have a class that represents that object?

public TagElement
{
    private XElement tagXml
    public string Name
    public DateTime LastClocked
    public DateTime TotalClocked
}

Now I'd simply make each of those properties point into the private XML object get / set their values.

public TagElement(string name, DateTime initialValue)
{
    tagXml = new XElement("Tag",
                 new XAttribute("Name", name),
                 new XElement("LastClocked", initialValue),
                 new XElement("TotalClocked", initialValue));
}

public DateTime LastClocked
{
    get
    {
        return tagXml.Descendents("LastClocked").Single().Value;
    }
    set
    {
        tagXml.Descendents("LastClocked").Single() = value;
    }
}

You can still have a Save method that persists the XElement to a file.  You probably also want a collection class that wraps the individual TagElements.

public TagCollection
{
    public List<TagElement> TagElements { get; set; }
}

So you can then use

var collection = new TagCollection("myFile.xml");
// constructor for TagCollection needs to parse the XML, and create TagElements

var fooElement = collection.TagElements.Single(tag => tag.Name == "foo");
fooElement.LastClocked = DateTime.Now;

collection.TagElements.ForEach(tag => tag.TotalClocked = DateTime.Now);

In summary, prefer to use properties to get and set values than using methods.  Abstract away the XML so that when you're using the class you do not know or care that it is being persisted to XML.


Recommend Next Note Purchase I have just started trying to learn Haskell and tried to think of a practical, but simple problem to try and solve.  I have a strong background in C, but am just beginning to dip my toes in the world of functional programming.

The idea came from my investing in notes on Prosper: given a list of note ratings, and a desired distribution, what should the next note purchase be, in order to most align with the desired distribution.

Here's an example:

Say I currently own 5 notes of an "A" rating, and 4 notes of a "B" rating.  If my desired distribution is 50% "A" and 50% "B", then the next note purchased should be of a "B" rating. However, if my desired distribution was 90/10, the next note should be an "A" rating.

The below code seems to work (albeit with no error checking), but feels very unreadable.  What are some ways I could refactor to make the code more readable or idiomatic to Haskell?

module Main where

import Data.List
import Data.Ord

main::IO()
main = print $ recommendNote [ "B", "C", "D", "D", "HR" ]  [ 0, 0, 0, 0.5, 0.25, 0.25, 0]

noteTypes :: [String]
noteTypes = [ "AA", "A", "B", "C", "D", "E", "HR" ]

recommendNote :: (Ord a, Fractional a) => [String] -> [a] -> String
recommendNote notes targetDist = let y = zip (subtractLists targetDist (getDistribution notes)) noteTypes
                                in snd (maximumBy (comparing fst) y)

subtractLists :: (Num a) => [a] -> [a] -> [a]
subtractLists = zipWith (-)

count :: [String] -> String -> Int
count inList x =  length $ filter (x==) inList

getNoteCount :: [String] -> [Int]
getNoteCount inList = map (count inList) noteTypes

getDistribution :: (Fractional a) => [String] -> [a]
getDistribution inList = percentize $ map fromIntegral (getNoteCount inList)

percentize :: (Fractional a) => [a] -> [a]
percentize inList = map (/ sum inList) inList

Order of arguments

As a convention, arguments in functions are always ordered from most likely to change to less likely to change.

Because this allows...

Simplification (Currying)

Haskell is like Math.

Look at

2x + x = 1 + x

You would like the simplify the x, so:

2x = 1

The same applies to your code:

percentize inList = map (/ sum inList) inList

You can omit inList as it is just repeated on both sides:

count x inList =  length $ filter (x==) inList

Becomes:

count x = length . filter (x==)

Now the focus is not on the transformation of the argument, but on the definition of a function as the successive application of two (or more) known ones.

Another example is:

getDistribution inList = percentize $ map fromIntegral (getNoteCount inList)

That becomes:

getDistribution = percentize . map fromIntegral . getNoteCount

This is, left to right, a description of what the function does.

Conventional names

I see that inList goes inside the function, and I can see that it is a List from the type signature, such long names are not needed.

Just use the conventional xs for general lists:

percentize xs = map (/ sum xs) xs

Use where not let

It is more natural to read from general to specific than the opposite:

recommendNote notes targetDist = let y = zip (subtractLists targetDist (getDistribution notes)) noteTypes
                                in snd (maximumBy (comparing fst) y)

Becomes:

recommendNote notes targetDist = snd (maximumBy (comparing fst) y)
  where
    y = zip (subtractLists targetDist (getDistribution notes)) noteTypes

Where you also avoid overly long lines.

Generality

Be optimistic and general with your types:

count :: String -> [String] -> Int

Becomes:

count :: a -> [a] -> Int

Where a stands for anything.

Not everything is get

get is a specific OO technical term, prefixing it before the function names is confusing, just drop it.

Make tiny functions local

Things like:

subtractLists = zipWith (-)

May be local and without a type declaration, like:

recommendNote notes targetDist = snd (maximumBy (comparing fst) y)
  where
    y = zip (subtractLists targetDist (getDistribution notes)) noteTypes
    subtractLists = zipWith (-)


Pythonic way to count if a value is 'NaN' in a dictionary of dictionaries I have a dictionary of dictionaries with several values. I want to count all the entries that do not contain 'NaN' (Not a Number in a string).

for each in enron_data:
    if enron_data[each]["salary"] !='NaN':
        counter += 1

    if enron_data[each]['email_address']!='NaN':
        valid_email +=1

I'm not familiar with list comprehensions, but I'm pretty sure there's might be a more pythonic way to achieve this.

Can anyone share pythonic advice?

Thank you

If you want this to use a comprehension, you need to get the sum of each each in enron_data, where eachs salary is not 'NaN'. As highlighted by the word exercise, you should notice that each is probably not the best variable name for this. Take the following example:

  you need to get the sum of each boat in boats, where the boats cost is not 'NaN'.

Which is easier to read. With Python it's the same, and is why good variable names are advised.

And so you'd want to do:

counter += sum(enron_data[each]["salary"] !='NaN' for each in enron_data)
valid_email += sum(enron_data[each]["email_address"] !='NaN' for each in enron_data)

If enron_data has a function like dict.values, or better dict.itervalues, then you'd want to use that instead. The latter one is simply a version of the former with better memory usage.

And so you could instead use:

counter += sum(each["salary"] !='NaN' for each in enron_data.itervalues())
valid_email += sum(each["email_address"] !='NaN' for each in enron_data.itervalues())

I don't think this approach is that much better than your current, but you could make it a function, to reduce code duplication, but whether it's better ultimately comes down to how you're using it.

But it's definitely better than the answer you provided. There's no need to create a new dictionary, it changes memory usage from \$O(1)\$ to \$O(n)\$, is harder to read, and is slower, as it requires more effort to create a dictionary, then to sum booleans.


Find k pairs with smallest sums in Python I'm trying to improve my Python, so I'm trying to do coding challenges and getting them reviewed. I would really appreciate if someone can point out how I can make my code cleaner/more pythonic. My min_heap with a 3-value tuple seems a bit messy, but I couldn't think of a better way to do it.

Problem:

https://leetcode.com/problems/find-k-pairs-with-smallest-sums/description/

  You are given two integer arrays nums1 and nums2 sorted in ascending
  order and an integer k.

  Define a pair (u,v) which consists of one element from the first array and one element from the second array.

  Find the k pairs (u1,v1),(u2,v2) ...(uk,vk) with the smallest sums.

Example 1:

  Given nums1 = [1,7,11], nums2 = [2,4,6],  k = 3

  Return: [1,2],[1,4],[1,6]

  The first 3 pairs are returned from the sequence:
      [1,2],[1,4],[1,6],[7,2],[7,4],[11,2],[7,6],[11,4],[11,6]

Solution:

from heapq import heappop, heappush

class Solution:
    def kSmallestPairs(self, nums1, nums2, k):
        """
        :type nums1: List[int]
        :type nums2: List[int]
        :type k: int
        :rtype: List[List[int]]
        """
        solution = []
        min_heap = []

        # if nums1 or nums2 is empty or k is 0, we're done
        if not nums1 or not nums2 or not k:
            return solution

        # first initialize min_heap with all
        # (nums1[0..n], nums2[0]) pairs with
        # (sum of pair, (nums1[0..n], nums2[0]), index of nums2)
        for value in nums1:
            heappush(min_heap, (value+nums2[0], (value, nums2[0]), 0))

        while k and min_heap:
            curr_pair = heappop(min_heap)
            solution.append(curr_pair[1])
            nums2_idx = curr_pair[2]
            k -= 1
            # if we haven't exhausted all (nums1[curr], nums2[0..n])
            # pairs, offer pair (nums1[curr], nums2[curr+1]) to min_heap
            if curr_pair[2] == len(nums2)-1: continue

            heappush(min_heap, (curr_pair[1][0]+nums2[nums2_idx+1], (curr_pair[1][0],
                nums2[nums2_idx+1]), nums2_idx+1))

        return solution

There's no need for the special case not k, because that is already handled by the while k in the main loop.
In the initial loop over nums1 the expression nums2[0] gets computed twice. This could be avoided by storing the result in a local variable:

value2 = nums2[0]
heappush(min_heap, (value + value2, (value, value2), 0))

In the main loop there are some complicated expressions, culminating in:

heappush(min_heap, (curr_pair[1][0]+nums2[nums2_idx+1], (curr_pair[1][0],
    nums2[nums2_idx+1]), nums2_idx+1))

which I found hard to understand. Complex expressions can be made clearer by giving names to the subexpressions. In particular, we can use tuple assignment to give names to all the elements of the tuple:

_, pair, j = heappop(min_heap)
value1, _ = pair

The reason for writing the tuple assignment value1, _ = pair instead of the lookup value1 = pair[0] is that the tuple assignment shows the reader that the pair has exactly two elements. (And if it doesn't then Python will raise "ValueError: not enough values to unpack" or "ValueError: too many values to unpack", so we get some checking too).
Now the heappush call looks like this:

heappush(min_heap, (value1 + nums2[j + 1], (value1, nums2[j + 1]), j + 1)

Here the expression nums2[j + 1] gets computed twice. This could be avoided by using a local variable:

value2 = nums2[j + 1]
heappush(min_heap, (value1 + value2, (value1, value2), j + 1)

Now, it ought to strike you that there is a strong similarity between the revised code in §1 and §4 above. The duplication could be avoided by defining a local function, like this:

solution = []
heap = []

def add(value1, j):
    # Add the pair (value1, nums2[j]) to the heap if possible.
    if j < len(nums2):
        value2 = nums2[j]
        heappush(heap, (value1 + value2, (value1, value2), j))

for value1 in nums1:
    add(value1, 0)

while k and heap:
    k -= 1
    _, pair, j = curr_pair = heappop(heap)
    solution.append(pair)
    value1, _ = pair
    add(value1, j + 1)

return solution

Note that in this version of the code there is no longer any need for the special cases not num1 and not nums2. If nums1 is empty, then there will be no iterations for for value1 in nums1. If nums2 is empty, then j < len(nums2) will always be false, and so nothing will be added to the heap. Either way, there will be no iterations of the main loop and so the solution will be empty as required.
The revised code in §5 above always takes at least as many steps as there are elements in nums1. But this is wasteful in many cases. Consider something like:

kSmallestPairs(list(range(1000000)), [1, 2, 3], 1)

Since k=1, then result should be the single pair [(0, 1)]. But the initialization loop runs over all the million elements of nums1.

This could be avoided by initializing the heap with the single pair (nums1[0], nums2[0]) since we know that this is the pair with the smallest sum. And then when we pop a pair (nums1[i], nums2[j]) from the heap, we have to add two new pairs to the heap: that is, (nums1[i + 1], nums2[j]) and (nums1[i], nums2[j + 1]), as either of these pairs might be next in order after the pair we just processed.

However, we have to be careful to add each pair just once — after processing (nums1[1], nums2[0]) and (nums1[0], nums2[1]) we must not add the pair (nums1[1], nums2[1]) twice. This can be avoided by keeping a set of all the pairs we have added to the heap.

solution = []
heap = []     # Min-heap of (nums1[i] + nums2[j], i, j)
added = set() # Set of indexes (i, j) that have been added to heap.

def add(i, j):
    # Add (nums1[i] + nums2[j], i, j) to the heap if possible.
    if i < len(nums1) and j < len(nums2) and (i, j) not in added:
        added.add((i, j))
        heappush(heap, (nums1[i] + nums2[j], i, j))

add(0, 0)
while k and heap:
    k -= 1
    _, i, j = heappop(heap)
    solution.append((nums1[i], nums2[j]))
    add(i + 1, j)
    add(i, j + 1)
return solution